Message ID | 1400878647-22176-1-git-send-email-mjt@msgid.tls.msk.ru |
---|---|
State | New |
Headers | show |
Michael Tokarev <mjt@tls.msk.ru> writes: > The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options() > has a weird bug in variable usage around expanding opts->vreader > array. > > There's a helper variable, vreaderOpt, which is first needlessly > initialized to NULL, next, conditionally, only we have to expand > opts->vreader, receives array expansion from g_renew() (initially > realloc), and next, even if we don't actually perform expansion, I don't get the "(initially realloc)" part. The sentence makes sense to me just fine without it, though. > the value of this variable is assigned to the actual array, > opts->vreader, which was supposed to be expanded. > > So, since we expand the array by READER_STEP increments, only > once in READER_STEP (=4) the code will work, in other 3/4 times > it will fail badly. > > Fix this by not using this temp variable when expanding the > array, and by dropping the useless =NULL initializer too - > if it wasn't in place initially, compiler warned us about "would have warned us"? > this problem at the beginning. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > libcacard/vcard_emul_nss.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c > index b7db51d..8462aef 100644 > --- a/libcacard/vcard_emul_nss.c > +++ b/libcacard/vcard_emul_nss.c > @@ -1149,7 +1149,7 @@ vcard_emul_options(const char *args) > char type_str[100]; > VCardEmulType type; > int count, i; > - VirtualReaderOptions *vreaderOpt = NULL; > + VirtualReaderOptions *vreaderOpt; > > args = strip(args + 5); > if (*args != '(') { > @@ -1173,11 +1173,10 @@ vcard_emul_options(const char *args) > > if (opts->vreader_count >= reader_count) { > reader_count += READER_STEP; > - vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader, > - reader_count); > + opts->vreader = g_renew(VirtualReaderOptions, opts->vreader, > + reader_count); > } > - opts->vreader = vreaderOpt; > - vreaderOpt = &vreaderOpt[opts->vreader_count]; > + vreaderOpt = &opts->vreader[opts->vreader_count]; > vreaderOpt->name = g_strndup(name, name_length); > vreaderOpt->vname = g_strndup(vname, vname_length); > vreaderOpt->card_type = type; Much more straightforward now. Thanks! Reviewed-by: Markus Armbruster <armbru@redhat.com>
26.05.2014 10:25, Markus Armbruster wrote: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options() >> has a weird bug in variable usage around expanding opts->vreader >> array. >> >> There's a helper variable, vreaderOpt, which is first needlessly >> initialized to NULL, next, conditionally, only we have to expand >> opts->vreader, receives array expansion from g_renew() (initially >> realloc), and next, even if we don't actually perform expansion, > > I don't get the "(initially realloc)" part. The sentence makes sense to > me just fine without it, though. I was in context of your patch which changes realloc() to g_renew(). And I failed to mention that this my patch is on top of yuors, too - think of this comment as such a mention ;) >> the value of this variable is assigned to the actual array, >> opts->vreader, which was supposed to be expanded. >> >> So, since we expand the array by READER_STEP increments, only >> once in READER_STEP (=4) the code will work, in other 3/4 times >> it will fail badly. >> >> Fix this by not using this temp variable when expanding the >> array, and by dropping the useless =NULL initializer too - >> if it wasn't in place initially, compiler warned us about > > "would have warned us"? Oh yeah. I tried to remember the right English construct, but failed. This tense always escpapes my mind for some reason :) > Much more straightforward now. Thanks! > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thank you. I'll fix the comment. And I'm now ready to push whole -trivial. /mjt
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c index b7db51d..8462aef 100644 --- a/libcacard/vcard_emul_nss.c +++ b/libcacard/vcard_emul_nss.c @@ -1149,7 +1149,7 @@ vcard_emul_options(const char *args) char type_str[100]; VCardEmulType type; int count, i; - VirtualReaderOptions *vreaderOpt = NULL; + VirtualReaderOptions *vreaderOpt; args = strip(args + 5); if (*args != '(') { @@ -1173,11 +1173,10 @@ vcard_emul_options(const char *args) if (opts->vreader_count >= reader_count) { reader_count += READER_STEP; - vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader, - reader_count); + opts->vreader = g_renew(VirtualReaderOptions, opts->vreader, + reader_count); } - opts->vreader = vreaderOpt; - vreaderOpt = &vreaderOpt[opts->vreader_count]; + vreaderOpt = &opts->vreader[opts->vreader_count]; vreaderOpt->name = g_strndup(name, name_length); vreaderOpt->vname = g_strndup(vname, vname_length); vreaderOpt->card_type = type;
The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options() has a weird bug in variable usage around expanding opts->vreader array. There's a helper variable, vreaderOpt, which is first needlessly initialized to NULL, next, conditionally, only we have to expand opts->vreader, receives array expansion from g_renew() (initially realloc), and next, even if we don't actually perform expansion, the value of this variable is assigned to the actual array, opts->vreader, which was supposed to be expanded. So, since we expand the array by READER_STEP increments, only once in READER_STEP (=4) the code will work, in other 3/4 times it will fail badly. Fix this by not using this temp variable when expanding the array, and by dropping the useless =NULL initializer too - if it wasn't in place initially, compiler warned us about this problem at the beginning. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- libcacard/vcard_emul_nss.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)