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 <[email protected]> >> Reviewed-by: Alon Levy <[email protected]> >> --- >> 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
