[Skiboot] [PATCH] nvram: Fix a possible NULL pointer de-ref in nvram_query_eq()

[Vaibhav Jain]

Thanks for looking into this patch Oliver.

Oliver <oohall at gmail.com> writes:

> This isn't a valid use of the API. nvram_query_eq() is just a helper
> for the common case where there's only one valid value for that key
> (e.g. fast-reset=0). If you're doing anything more complex you should
> be using nvram_query() to retrieve the value (which is NULL if the key
> doesn't exist) and checking each case manually.

I looked at existing code and the commit description and inferred that:

Since:
        nvram_query_eq(key,value) <=> (nvram_query(key) == value)
hence,
        nvram_query_eq(key,NULL) <=> (nvram_query(key) == NULL)

For keys which are essentially boolean flags this makes it easy to
check if a given key is not defined in the nvram and. So, I dont
understand why it should not be a valid use of the API.
The proposed change makes the function more general and protects it
against an invalid NULL arg.

>
>> Signed-off-by: Vaibhav Jain <vaibhav at linux.ibm.com>
>> ---
>>  core/nvram-format.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/core/nvram-format.c b/core/nvram-format.c
>> index 42c5cbbb..c52b443a 100644
>> --- a/core/nvram-format.c
>> +++ b/core/nvram-format.c
>> @@ -282,8 +282,8 @@ bool nvram_query_eq(const char *key, const char *value)
>>  {
>>         const char *s = nvram_query(key);
>>
>> -       if (!s)
>> -               return false;
>> +       if (s == NULL || value == NULL)
>> +               return s == value;
>
> Put an assert(value) before the strcmp and leave it at that. Maybe add
> comment about the intended usage of nvram_query_eq()

-- 
Vaibhav Jain <vaibhav at linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.