[dpdk-dev,v3,5/9] mem: fix potential resource leak
Checks
Commit Message
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
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
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.
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
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
>
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?
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.
@@ -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);