[v2,6/7] libcacard/vcard_emul_nss: Assert vreaderOpt isn't null

It's not locally obvious, and Coverity can't see it either.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcard_emul_nss.c | 1 +
 1 file changed, 1 insertion(+)

23.05.2014 15:24, Markus Armbruster пишет:
> It's not locally obvious, and Coverity can't see it either.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Alon Levy <alevy@redhat.com>
> ---
>  libcacard/vcard_emul_nss.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index 2048917..4f55e44 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>                                       reader_count);
>              }
> +            assert(vreaderOpt);
>              opts->vreader = vreaderOpt;
>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>              vreaderOpt->name = g_strndup(name, name_length);

Shouldn't the assignment be moved up one line into the if {}
statement instead?

Sigh, thats a second comment about this code... :)

Thanks,

/mjt

23.05.2014 22:09, Michael Tokarev wrote:
> 23.05.2014 15:24, Markus Armbruster wrote:
>> It's not locally obvious, and Coverity can't see it either.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Alon Levy <alevy@redhat.com>
>> ---
>>  libcacard/vcard_emul_nss.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
>> index 2048917..4f55e44 100644
>> --- a/libcacard/vcard_emul_nss.c
>> +++ b/libcacard/vcard_emul_nss.c
>> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>>                                       reader_count);
>>              }
>> +            assert(vreaderOpt);
>>              opts->vreader = vreaderOpt;
>>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>>              vreaderOpt->name = g_strndup(name, name_length);
> 
> Shouldn't the assignment be moved up one line into the if {}
> statement instead?

Actually it looks like this code is just buggy, it works for just
one iteration.  Because at this place, vreaderOpts will be non-NULL
only if we expanded the array.  If we didn't (we do that in
READER_STEP increments), we'll be assigning NULL to opts->vreader,
because vreaderOpt will _really_ be NULL here.

One good example when setting initial values like this (for vreaderOpts)
is a Bad Idea (tm).  If it weren't needlessly NULL initially, compiler
was able to tell us about using uninitialized variable.

Thanks,

/mjt

23.05.2014 22:16, Michael Tokarev пишет:
> 23.05.2014 22:09, Michael Tokarev wrote:
>> 23.05.2014 15:24, Markus Armbruster wrote:
>>> It's not locally obvious, and Coverity can't see it either.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Alon Levy <alevy@redhat.com>
>>> ---
>>>  libcacard/vcard_emul_nss.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
>>> index 2048917..4f55e44 100644
>>> --- a/libcacard/vcard_emul_nss.c
>>> +++ b/libcacard/vcard_emul_nss.c
>>> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>>>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>>>                                       reader_count);
>>>              }
>>> +            assert(vreaderOpt);
>>>              opts->vreader = vreaderOpt;
>>>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>>>              vreaderOpt->name = g_strndup(name, name_length);
>>
>> Shouldn't the assignment be moved up one line into the if {}
>> statement instead?
> 
> Actually it looks like this code is just buggy, it works for just
> one iteration.  Because at this place, vreaderOpts will be non-NULL
> only if we expanded the array.  If we didn't (we do that in
> READER_STEP increments), we'll be assigning NULL to opts->vreader,
> because vreaderOpt will _really_ be NULL here.

So, the real fix is:

1) drop = NULL at declaration of vreaderOpt;
2) do not mention vreaderOpt inside the if{} statement at
   all, we don't need indirection there;
3) drop this opts->vreader assignment

and ofcourse do not add the assert as in the patch above ;)

/mjt

> One good example when setting initial values like this (for vreaderOpts)
> is a Bad Idea (tm).  If it weren't needlessly NULL initially, compiler
> was able to tell us about using uninitialized variable.
> 
> Thanks,
> 
> /mjt
>

Michael Tokarev <mjt@tls.msk.ru> writes:

> 23.05.2014 22:16, Michael Tokarev пишет:
>> 23.05.2014 22:09, Michael Tokarev wrote:
>>> 23.05.2014 15:24, Markus Armbruster wrote:
>>>> It's not locally obvious, and Coverity can't see it either.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> Reviewed-by: Alon Levy <alevy@redhat.com>
>>>> ---
>>>>  libcacard/vcard_emul_nss.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
>>>> index 2048917..4f55e44 100644
>>>> --- a/libcacard/vcard_emul_nss.c
>>>> +++ b/libcacard/vcard_emul_nss.c
>>>> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>>>>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>>>>                                       reader_count);
>>>>              }
>>>> +            assert(vreaderOpt);
>>>>              opts->vreader = vreaderOpt;
>>>>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>>>>              vreaderOpt->name = g_strndup(name, name_length);
>>>
>>> Shouldn't the assignment be moved up one line into the if {}
>>> statement instead?
>> 
>> Actually it looks like this code is just buggy, it works for just
>> one iteration.  Because at this place, vreaderOpts will be non-NULL
>> only if we expanded the array.  If we didn't (we do that in
>> READER_STEP increments), we'll be assigning NULL to opts->vreader,
>> because vreaderOpt will _really_ be NULL here.

You're right.  When I saw the conditional realloc, I jumped to convince
myself that it's always executed in the first loop iteration, but missed
the fact that vreaderOpt is reset to null on *every* iteration.

> So, the real fix is:
>
> 1) drop = NULL at declaration of vreaderOpt;
> 2) do not mention vreaderOpt inside the if{} statement at
>    all, we don't need indirection there;
> 3) drop this opts->vreader assignment
>
> and ofcourse do not add the assert as in the patch above ;)

I'll review it.  Thanks!