[dpdk-dev,v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

  Check the FILE *f and rte_mempool *mp pointers for NULL and
return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.

Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
---
 lib/librte_mempool/rte_mempool.c | 3 +++
 1 file changed, 3 insertions(+)
  

On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> 
> Check the FILE *f and rte_mempool *mp pointers for NULL and
> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> 
> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> ---
>  lib/librte_mempool/rte_mempool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 332f469..0f71f10 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -765,6 +765,9 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>     unsigned common_count;
>     unsigned cache_count;
> 
> +   RTE_VERIFY(f != NULL);
> +   RTE_VERIFY(mp != NULL);
> +
>     fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>     fprintf(f, "  flags=%x\n", mp->flags);
>     fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> -- 
> 2.1.0
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> 

I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
macro here instead using what I suggested in my other note
Neil
  

On Sep 28, 2014, at 7:27 AM, Neil Horman <nhorman@tuxdriver.com> wrote:

> On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
>> 
>> Check the FILE *f and rte_mempool *mp pointers for NULL and
>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
>> 
>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>> ---
>> lib/librte_mempool/rte_mempool.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 332f469..0f71f10 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -765,6 +765,9 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>>    unsigned common_count;
>>    unsigned cache_count;
>> 
>> +   RTE_VERIFY(f != NULL);
>> +   RTE_VERIFY(mp != NULL);
>> +
>>    fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>>    fprintf(f, "  flags=%x\n", mp->flags);
>>    fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
>> -- 
>> 2.1.0
>> 
>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>> 
>> 
> 
> I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> macro here instead using what I suggested in my other note
> Neil

Maybe I can add RTE_VERIFY_WARN() later or someone else can.
> 

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
  

2014-09-28 08:27, Neil Horman:
> On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > 
> > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> 
> I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> macro here instead using what I suggested in my other note

Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
If you look elsewhere in the DPDK code, you'll see that it's not common to do
such check on input parameters.
A similar discussion already happened few months ago:
	http://dpdk.org/ml/archives/dev/2014-June/003900.html
  

On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> 2014-09-28 08:27, Neil Horman:
> > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > 
> > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > 
> > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > macro here instead using what I suggested in my other note
> 
> Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> If you look elsewhere in the DPDK code, you'll see that it's not common to do
> such check on input parameters.
> A similar discussion already happened few months ago:
> 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> 
Not sure what your point is here Thomas.  I think we're all in agreement that
NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
with a NULL check at all and just accept the crash as it is?

Neil

> -- 
> Thomas
>
  

On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > 2014-09-28 08:27, Neil Horman:
> > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > 
> > > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > > 
> > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > macro here instead using what I suggested in my other note
> > 
> > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > such check on input parameters.
> > A similar discussion already happened few months ago:
> > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > 
> Not sure what your point is here Thomas.  I think we're all in agreement that
> NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> with a NULL check at all and just accept the crash as it is?
>

In the general case:
* Code in the datapath should not have things like NULL checks
* However, datapath code should generally have a debug option which turns 
  these checks on to help debugging if needed. 
* Code not in the datapath probably should have these checks.

My 2c here

/Bruce
  

On Oct 1, 2014, at 10:43 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
>> On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
>>> 2014-09-28 08:27, Neil Horman:
>>>> On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
>>>>> Check the FILE *f and rte_mempool *mp pointers for NULL and
>>>>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
>>>>> 
>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
>>>> 
>>>> I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
>>>> thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
>>>> macro here instead using what I suggested in my other note
>>> 
>>> Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
>>> If you look elsewhere in the DPDK code, you'll see that it's not common to do
>>> such check on input parameters.
>>> A similar discussion already happened few months ago:
>>> 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
>>> 
>> Not sure what your point is here Thomas.  I think we're all in agreement that
>> NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
>> with a NULL check at all and just accept the crash as it is?
>> 
> 
> In the general case:
> * Code in the datapath should not have things like NULL checks
> * However, datapath code should generally have a debug option which turns 
>  these checks on to help debugging if needed. 
> * Code not in the datapath probably should have these checks.

IMO the last point is the reason for the rte_mempool_dump() check. We could always add a new macro (if not already defined) that could be compiled out if not enabled as a debug. I would not want to put ‘ifdef’ around that code but include the ifdef in the macro/header file. Removing ifdefs from the code .c files should be the long term goal as a side note.
> 
> My 2c here
> 
> /Bruce

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
  

On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > 2014-09-28 08:27, Neil Horman:
> > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > 
> > > > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > > > 
> > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > macro here instead using what I suggested in my other note
> > > 
> > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > such check on input parameters.
> > > A similar discussion already happened few months ago:
> > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > 
> > Not sure what your point is here Thomas.  I think we're all in agreement that
> > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > with a NULL check at all and just accept the crash as it is?
> >
> 
> In the general case:
> * Code in the datapath should not have things like NULL checks
> * However, datapath code should generally have a debug option which turns 
>   these checks on to help debugging if needed. 
> * Code not in the datapath probably should have these checks.
> 
Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
datapath, its rather by definition a debug function, isn't it?
Neil

> My 2c here
> 
> /Bruce
>
  

On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > 2014-09-28 08:27, Neil Horman:
> > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > 
> > > > > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > > > > 
> > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > macro here instead using what I suggested in my other note
> > > > 
> > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > such check on input parameters.
> > > > A similar discussion already happened few months ago:
> > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > 
> > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > with a NULL check at all and just accept the crash as it is?
> > >
> > 
> > In the general case:
> > * Code in the datapath should not have things like NULL checks
> > * However, datapath code should generally have a debug option which turns 
> >   these checks on to help debugging if needed. 
> > * Code not in the datapath probably should have these checks.
> > 
> Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> datapath, its rather by definition a debug function, isn't it?
> Neil

Yes, agreed.  [So it probably should have the NULL check].
/Bruce
  

2014-10-01 17:05, Bruce Richardson:
> On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > > 2014-09-28 08:27, Neil Horman:
> > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > > 
> > > > > > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > > > > > 
> > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > > macro here instead using what I suggested in my other note
> > > > > 
> > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > > such check on input parameters.
> > > > > A similar discussion already happened few months ago:
> > > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > > 
> > > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > > with a NULL check at all and just accept the crash as it is?
> > > >
> > > 
> > > In the general case:
> > > * Code in the datapath should not have things like NULL checks
> > > * However, datapath code should generally have a debug option which turns 
> > >   these checks on to help debugging if needed. 
> > > * Code not in the datapath probably should have these checks.
> > > 
> > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> > datapath, its rather by definition a debug function, isn't it?
> > Neil
> 
> Yes, agreed.  [So it probably should have the NULL check].

I have many arguments to not do this check:
1) If it was a coding rule to do this kind of check, it should be done in
almost every functions.
2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)?
3) Why check only NULL value? 1 and 2 are also some invalid values...
  

On Thu, Oct 02, 2014 at 09:47:19AM +0200, Thomas Monjalon wrote:
> 2014-10-01 17:05, Bruce Richardson:
> > On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> > > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > > > 2014-09-28 08:27, Neil Horman:
> > > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > > > 
> > > > > > > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > > > > > > 
> > > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > > > macro here instead using what I suggested in my other note
> > > > > > 
> > > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > > > such check on input parameters.
> > > > > > A similar discussion already happened few months ago:
> > > > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > > > 
> > > > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > > > with a NULL check at all and just accept the crash as it is?
> > > > >
> > > > 
> > > > In the general case:
> > > > * Code in the datapath should not have things like NULL checks
> > > > * However, datapath code should generally have a debug option which turns 
> > > >   these checks on to help debugging if needed. 
> > > > * Code not in the datapath probably should have these checks.
> > > > 
> > > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> > > datapath, its rather by definition a debug function, isn't it?
> > > Neil
> > 
> > Yes, agreed.  [So it probably should have the NULL check].
> 
> I have many arguments to not do this check:
> 1) If it was a coding rule to do this kind of check, it should be done in
> almost every functions.
Only if NULL is an invalid value, and we spot check for NULL all the time (see
eal_parse_coremask as an example from a quick search).

> 2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)?
Its also quite common to do the check.  I think this is more about if it makes
sense to do it here (i.e. is it a common error to pass a NULL pointer into
mempool_dump?).  If so, an extra check with its own specific panic might be
nice.

> 3) Why check only NULL value? 1 and 2 are also some invalid values...
> 
Because NULL is the common case.
Neil

> -- 
> Thomas
>
  

2014-10-02 07:37, Neil Horman:
> On Thu, Oct 02, 2014 at 09:47:19AM +0200, Thomas Monjalon wrote:
> > 2014-10-01 17:05, Bruce Richardson:
> > > On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> > > > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > > > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > > > > 2014-09-28 08:27, Neil Horman:
> > > > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> > > > > > > > 
> > > > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > > > > macro here instead using what I suggested in my other note
> > > > > > > 
> > > > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > > > > such check on input parameters.
> > > > > > > A similar discussion already happened few months ago:
> > > > > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > > > > 
> > > > > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > > > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > > > > with a NULL check at all and just accept the crash as it is?
> > > > > >
> > > > > 
> > > > > In the general case:
> > > > > * Code in the datapath should not have things like NULL checks
> > > > > * However, datapath code should generally have a debug option which turns 
> > > > >   these checks on to help debugging if needed. 
> > > > > * Code not in the datapath probably should have these checks.
> > > > > 
> > > > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> > > > datapath, its rather by definition a debug function, isn't it?
> > > > Neil
> > > 
> > > Yes, agreed.  [So it probably should have the NULL check].
> > 
> > I have many arguments to not do this check:
> > 1) If it was a coding rule to do this kind of check, it should be done in
> > almost every functions.
> Only if NULL is an invalid value, and we spot check for NULL all the time (see
> eal_parse_coremask as an example from a quick search).
> 
> > 2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)?
> Its also quite common to do the check.  I think this is more about if it makes
> sense to do it here (i.e. is it a common error to pass a NULL pointer into
> mempool_dump?).  If so, an extra check with its own specific panic might be
> nice.
> 
> > 3) Why check only NULL value? 1 and 2 are also some invalid values...
> > 
> Because NULL is the common case.
> Neil

Argumentation accepted and patch applied.