[dpdk-stable] [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Akhil Goyal
> Sent: Friday, April 27, 2018 12:59 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; Akhil Goyal
> <akhil.goyal at nxp.com>; Zhang, Roy Fan <roy.fan.zhang at intel.com>
> Cc: dev at dpdk.org; stable at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after
> freeing
> 
> Hi Pablo,
> 
> On 4/27/2018 5:06 PM, De Lara Guarch, Pablo wrote:
> > Hi Akhil,
> >
> >> -----Original Message-----
> >> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> >> Sent: Friday, April 27, 2018 9:47 AM
> >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; Zhang, Roy Fan
> >> <roy.fan.zhang at intel.com>
> >> Cc: dev at dpdk.org; stable at dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer
> after
> >> freeing
> >>
> >> Hi Pablo,
> >>
> >> On 4/26/2018 8:39 PM, Pablo de Lara wrote:
> >>> When freeing memory, pointers should be set to NULL, to avoid memory
> >>> corruption/segmentation faults.
> >>
> >> Shouldn't this be handled in the rte_free itself. A lot of other driver are
> also not
> >> setting null after rte_free.
> >> This would require change at a lot of places if this is not handled in
> rte_free.
> >>
> >
> > The glibc function "free" works the same way. Users are responsible for
> > setting to NULL these pointers (because sometimes, it is not necessary to do
> such thing).
> Yes it is correct but rte_free is custom free API in DPDK which can be
> modified or we can have a safer API rte_free_safe which can set the
> pointer to null.
> >
> > Anyway, in case we still wanted to change it, we would need to pass a
> pointer
> > to a pointer in rte_free, which would imply an API breakage.


Actually there is an alternative solution, by creating a macro like so;

#define rte_free(x) do {
     rte_free_(x); /* call the real implementation, now with _ */
     x = NULL;
} while (0)

This is not an ABI break if symbol versioning is used for rte_free().

It is an API change however - not that the calling code has to change,
but rather that the effect of rte_free() changes transparently.

I'm not sure what the correct thing to do is in this case - just pointing
out that this is another possible solution.


> I think if the community agrees, we can add this change may be in next
> releases.

+1 to discuss this with the community, regardless of the implementation :)


> > Thanks,
> > Pablo
> >
> >> Thanks,
> >> Akhil