diff mbox

[dpdk-dev,v3,3/9] mem: fix potential double close

Message ID baec1c09cf4e7db60f8c6c85d81587b6409e70d3.1524650130.git.anatoly.burakov@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply patch file failure

Commit Message

Burakov, Anatoly April 25, 2018, 9:56 a.m. UTC 
  We were closing descriptor before checking if mapping has
failed, but if it did, we did a second close afterwards. Fix
it by moving closing descriptor to after we check if mmap has
succeeded.

Coverity issue: 272560

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Bruce Richardson April 27, 2018, 3:15 p.m. UTC | #1 
On Wed, Apr 25, 2018 at 10:56:41AM +0100, Anatoly Burakov wrote:
> We were closing descriptor before checking if mapping has
> failed, but if it did, we did a second close afterwards. Fix
> it by moving closing descriptor to after we check if mmap has
> succeeded.
> 
> Coverity issue: 272560
> 
> Fixes: 2a04139f66b4 ("eal: add single file segments option")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Is a better fix not to assign fd to -1 after closing and then checking that
in the error leg?
Burakov, Anatoly April 27, 2018, 4:56 p.m. UTC | #2 
On 27-Apr-18 4:15 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:41AM +0100, Anatoly Burakov wrote:
>> We were closing descriptor before checking if mapping has
>> failed, but if it did, we did a second close afterwards. Fix
>> it by moving closing descriptor to after we check if mmap has
>> succeeded.
>>
>> Coverity issue: 272560
>>
>> Fixes: 2a04139f66b4 ("eal: add single file segments option")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
> Is a better fix not to assign fd to -1 after closing and then checking that
> in the error leg?
> 

A betterer fix would've been to move close() to until after all errors 
are checked. Will do that instead :)
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 1f553dd..9156f8b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -458,9 +458,6 @@  alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	 */
 	void *va = mmap(addr, alloc_sz, PROT_READ | PROT_WRITE,
 			MAP_SHARED | MAP_POPULATE | MAP_FIXED, fd, map_offset);
-	/* for non-single file segments, we can close fd here */
-	if (!internal_config.single_file_segments)
-		close(fd);
 
 	if (va == MAP_FAILED) {
 		RTE_LOG(DEBUG, EAL, "%s(): mmap() failed: %s\n", __func__,
@@ -471,6 +468,9 @@  alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
 		goto mapped;
 	}
+	/* for non-single file segments, we can close fd here */
+	if (!internal_config.single_file_segments)
+		close(fd);
 
 	rte_iova_t iova = rte_mem_virt2iova(addr);
 	if (iova == RTE_BAD_PHYS_ADDR) {