On Fri, Apr 27, 2018 at 12:37:08PM +0000, Van Haaren, Harry wrote: > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Akhil Goyal > > Sent: Friday, April 27, 2018 12:59 PM > > To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>; Akhil Goyal > > <akhil.go...@nxp.com>; Zhang, Roy Fan <roy.fan.zh...@intel.com> > > Cc: dev@dpdk.org; sta...@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.go...@nxp.com] > > >> Sent: Friday, April 27, 2018 9:47 AM > > >> To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>; Zhang, Roy > > >> Fan > > >> <roy.fan.zh...@intel.com> > > >> Cc: dev@dpdk.org; sta...@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
