diff mbox

[dpdk-dev,v3,4/9] mem: fix potential resource leak

Message ID 4d15c97e68ce89c0915935c6c04099a9eb950232.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

Anatoly Burakov April 25, 2018, 9:56 a.m. UTC 
  We close fd if we managed to find it in the list of allocated
segment lists (which should always be the case under normal
conditions), but if we didn't, the fd was leaking. Close it if
we couldn't find it in the segment list. This is not an issue
as if the segment is zero length, we're getting rid of it
anyway, so there's no harm in not storing the fd anywhere.

Coverity issue: 272568

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 | 2 ++
 1 file changed, 2 insertions(+)

Comments

Bruce Richardson April 27, 2018, 3:18 p.m. UTC | #1 
On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
> We close fd if we managed to find it in the list of allocated
> segment lists (which should always be the case under normal
> conditions), but if we didn't, the fd was leaking. Close it if
> we couldn't find it in the segment list. This is not an issue
> as if the segment is zero length, we're getting rid of it
> anyway, so there's no harm in not storing the fd anywhere.
> 
> Coverity issue: 272568
> 

This coverity issue indicates two resource leaks, while I think this patch
only closes one of them.

/Bruce

> 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 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> index 9156f8b..fab5a98 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> @@ -569,6 +569,8 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi,
>  			if (te != NULL && te->fd >= 0) {
>  				close(te->fd);
>  				te->fd = -1;
> +			} else {
> +				close(fd);
>  			}
>  			unlink(path);
>  		}
> -- 
> 2.7.4
Anatoly Burakov April 27, 2018, 3:28 p.m. UTC | #2 
On 27-Apr-18 4:18 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
>> We close fd if we managed to find it in the list of allocated
>> segment lists (which should always be the case under normal
>> conditions), but if we didn't, the fd was leaking. Close it if
>> we couldn't find it in the segment list. This is not an issue
>> as if the segment is zero length, we're getting rid of it
>> anyway, so there's no harm in not storing the fd anywhere.
>>
>> Coverity issue: 272568
>>
> 
> This coverity issue indicates two resource leaks, while I think this patch
> only closes one of them.
> 

You're right. Didn't notice this Coverity "feature"...
Anatoly Burakov April 27, 2018, 3:39 p.m. UTC | #3 
On 27-Apr-18 4:18 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
>> We close fd if we managed to find it in the list of allocated
>> segment lists (which should always be the case under normal
>> conditions), but if we didn't, the fd was leaking. Close it if
>> we couldn't find it in the segment list. This is not an issue
>> as if the segment is zero length, we're getting rid of it
>> anyway, so there's no harm in not storing the fd anywhere.
>>
>> Coverity issue: 272568
>>
> 
> This coverity issue indicates two resource leaks, while I think this patch
> only closes one of them.

The other issue is actually a false positive. We couldn't have gotten 
the fd if there wasn't a tailq entry for that fd, but if we don't resize 
and remove the file, we want to keep the fd. So the "int fd" goes out of 
scope, but actually it's stored in the tailq, and thus doesn't leak.
Bruce Richardson April 27, 2018, 3:48 p.m. UTC | #4 
On Fri, Apr 27, 2018 at 04:39:34PM +0100, Burakov, Anatoly wrote:
> On 27-Apr-18 4:18 PM, Bruce Richardson wrote:
> > On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
> > > We close fd if we managed to find it in the list of allocated
> > > segment lists (which should always be the case under normal
> > > conditions), but if we didn't, the fd was leaking. Close it if
> > > we couldn't find it in the segment list. This is not an issue
> > > as if the segment is zero length, we're getting rid of it
> > > anyway, so there's no harm in not storing the fd anywhere.
> > > 
> > > Coverity issue: 272568
> > > 
> > 
> > This coverity issue indicates two resource leaks, while I think this patch
> > only closes one of them.
> 
> The other issue is actually a false positive. We couldn't have gotten the fd
> if there wasn't a tailq entry for that fd, but if we don't resize and remove
> the file, we want to keep the fd. So the "int fd" goes out of scope, but
> actually it's stored in the tailq, and thus doesn't leak.
> 
I'm not sure coverity is going to recognise that fact. However, given that
this is a fix for one of the flagged problems:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 9156f8b..fab5a98 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -569,6 +569,8 @@  free_seg(struct rte_memseg *ms, struct hugepage_info *hi,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			unlink(path);
 		}