diff mbox

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

Message ID 36228cdd42eef261936b07c42a3c582f7e715da1.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 
  Normally, tailq entry should have a valid fd by the time we attempt
to map the segment. However, in case it doesn't, we're leaking fd,
so fix it.

Coverity issue: 272570

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:21 p.m. UTC | #1 
On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
> Normally, tailq entry should have a valid fd by the time we attempt
> to map the segment. However, in case it doesn't, we're leaking fd,
> so fix it.
> 
> Coverity issue: 272570
> 
> 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 fab5a98..b02e3a5 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>  			if (te != NULL && te->fd >= 0) {
>  				close(te->fd);
>  				te->fd = -1;

Is "fd" still not being leaked here, since we won't hit the else case and
then jump to the end of the function where it goes out of scope?

> +			} else {
> +				close(fd);
>  			}
>  			/* ignore errors, can't make it any worse */
>  			unlink(path);
> -- 
> 2.7.4
Burakov, Anatoly April 27, 2018, 3:49 p.m. UTC | #2 
On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
>> Normally, tailq entry should have a valid fd by the time we attempt
>> to map the segment. However, in case it doesn't, we're leaking fd,
>> so fix it.
>>
>> Coverity issue: 272570
>>
>> 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 fab5a98..b02e3a5 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>>   			if (te != NULL && te->fd >= 0) {
>>   				close(te->fd);
>>   				te->fd = -1;
> 
> Is "fd" still not being leaked here, since we won't hit the else case and
> then jump to the end of the function where it goes out of scope?

Technically, the "else" case is never valid here. If we have a tailq 
entry - we always have a valid fd. So perhaps it should be classified as 
a false positive.
Bruce Richardson April 27, 2018, 3:52 p.m. UTC | #3 
On Fri, Apr 27, 2018 at 04:49:03PM +0100, Burakov, Anatoly wrote:
> On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> > On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
> > > Normally, tailq entry should have a valid fd by the time we attempt
> > > to map the segment. However, in case it doesn't, we're leaking fd,
> > > so fix it.
> > > 
> > > Coverity issue: 272570
> > > 
> > > 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 fab5a98..b02e3a5 100644
> > > --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
> > >   			if (te != NULL && te->fd >= 0) {
> > >   				close(te->fd);
> > >   				te->fd = -1;
> > 
> > Is "fd" still not being leaked here, since we won't hit the else case and
> > then jump to the end of the function where it goes out of scope?
> 
> Technically, the "else" case is never valid here. If we have a tailq entry -
> we always have a valid fd. So perhaps it should be classified as a false
> positive.
> 
If there is a (non-harmful) way to fix it in the code, I'd definitely thing
we should do so. It means that any other projects which use coverity scans,
or other static analysis scans, on DPDK code won't have to re-disposition
the issue. Also, if we ever start having separate scans of the different
trees, we'll similarly see benefit of not having to mark false positives
multiple times.

/Bruce
Burakov, Anatoly April 27, 2018, 3:55 p.m. UTC | #4 
On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
>> Normally, tailq entry should have a valid fd by the time we attempt
>> to map the segment. However, in case it doesn't, we're leaking fd,
>> so fix it.
>>
>> Coverity issue: 272570
>>
>> 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 fab5a98..b02e3a5 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>>   			if (te != NULL && te->fd >= 0) {
>>   				close(te->fd);
>>   				te->fd = -1;
> 
> Is "fd" still not being leaked here, since we won't hit the else case and
> then jump to the end of the function where it goes out of scope?

Perhaps i should clarify - te->fd and fd are the same fd.

> 
>> +			} else {
>> +				close(fd);
>>   			}
>>   			/* ignore errors, can't make it any worse */
>>   			unlink(path);
>> -- 
>> 2.7.4
>
Bruce Richardson April 27, 2018, 4:27 p.m. UTC | #5 
On Fri, Apr 27, 2018 at 04:55:51PM +0100, Burakov, Anatoly wrote:
> On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> > On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
> > > Normally, tailq entry should have a valid fd by the time we attempt
> > > to map the segment. However, in case it doesn't, we're leaking fd,
> > > so fix it.
> > > 
> > > Coverity issue: 272570
> > > 
> > > 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 fab5a98..b02e3a5 100644
> > > --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
> > >   			if (te != NULL && te->fd >= 0) {
> > >   				close(te->fd);
> > >   				te->fd = -1;
> > 
> > Is "fd" still not being leaked here, since we won't hit the else case and
> > then jump to the end of the function where it goes out of scope?
> 
> Perhaps i should clarify - te->fd and fd are the same fd.
> 
Can you clarify that to coverity somehow?
Burakov, Anatoly April 27, 2018, 4:42 p.m. UTC | #6 
On 27-Apr-18 5:27 PM, Bruce Richardson wrote:
> On Fri, Apr 27, 2018 at 04:55:51PM +0100, Burakov, Anatoly wrote:
>> On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
>>> On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
>>>> Normally, tailq entry should have a valid fd by the time we attempt
>>>> to map the segment. However, in case it doesn't, we're leaking fd,
>>>> so fix it.
>>>>
>>>> Coverity issue: 272570
>>>>
>>>> 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 fab5a98..b02e3a5 100644
>>>> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>>>> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>>>>    			if (te != NULL && te->fd >= 0) {
>>>>    				close(te->fd);
>>>>    				te->fd = -1;
>>>
>>> Is "fd" still not being leaked here, since we won't hit the else case and
>>> then jump to the end of the function where it goes out of scope?
>>
>> Perhaps i should clarify - te->fd and fd are the same fd.
>>
> Can you clarify that to coverity somehow?
> 

I don't think i can. The fd comes from get_seg_fd(), which finds the 
tailq entry and either returns existing fd, or opens a new one - and the 
same tailq entry is later looked up by alloc_seg(). Technically, of 
course, tailq contents might change inbetween the calls, but really 
that's not possible as only one thread in any given process is ever 
running through this code.
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index fab5a98..b02e3a5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -524,6 +524,8 @@  alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			/* ignore errors, can't make it any worse */
 			unlink(path);