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

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Apr 27, 2018 at 12:37:08PM +0000, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
> > Sent: Friday, April 27, 2018 12:59 PM
> > To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal
> > <akhil.goyal@nxp.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>
> > Cc: dev@dpdk.org; stable@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@nxp.com]
> > >> Sent: Friday, April 27, 2018 9:47 AM
> > >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Zhang, Roy Fan
> > >> <roy.fan.zhang@intel.com>
> > >> Cc: dev@dpdk.org; stable@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 :)
> 
> 
I really don't think this change is necessary. I think having rte_free
behave as libc free is fine.

However, if we want to add a new API called rte_free_and_null(void **x),
I could be ok with that, though I'd be somewhat dubious of its necessity.
Static analysis tools should be able to pick up use-after-free errors,
though we may need to provide metadata to the tools in some form indicating
that rte_free is a free-ing function.

/Bruce