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 <arm...@redhat.com>
>>> Reviewed-by: Alon Levy <al...@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
>
