[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 6/7] libcacard/vcard_emul_nss: Assert vreader
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 6/7] libcacard/vcard_emul_nss: Assert vreaderOpt isn't null |
Date: |
Mon, 26 May 2014 08:16:37 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Michael Tokarev <address@hidden> 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 <address@hidden>
>>>> Reviewed-by: Alon Levy <address@hidden>
>>>> ---
>>>> 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!
[Qemu-devel] [PATCH v2 2/7] libcacard: Plug memory leaks around vreader_get_reader_list(), Markus Armbruster, 2014/05/23
[Qemu-devel] [PATCH v2 3/7] libcacard/vreader: Drop broken recovery from failed assertion, Markus Armbruster, 2014/05/23
[Qemu-devel] [PATCH v2 7/7] libcacard/vcard_emul_nss: Drop a redundant conditional, Markus Armbruster, 2014/05/23
[Qemu-devel] [PATCH v2 1/7] libcacard/vscclient: Bury some dead code, Markus Armbruster, 2014/05/23
[Qemu-devel] [PATCH v2 4/7] libcacard/vreader: Tighten assertion to clarify intent, Markus Armbruster, 2014/05/23
Re: [Qemu-devel] [PATCH v2 0/7] libcacard: A few simple fixes and cleanups, Michael Tokarev, 2014/05/23