| Message ID | 20170322204844.446-2-f4bug@amsat.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > static code analyzer complain: > > hw/usb/dev-smartcard-reader.c:816:5: warning: Null pointer passed as an argument to a 'nonnull' parameter > memcpy(p->abData, data, len); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Reported-by: Clang Static Analyzer > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/usb/dev-smartcard-reader.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c > index 757b8b3f5a..c38a4e5886 100644 > --- a/hw/usb/dev-smartcard-reader.c > +++ b/hw/usb/dev-smartcard-reader.c > @@ -799,8 +799,14 @@ static void ccid_write_parameters(USBCCIDState *s, CCID_Header *recv) > static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq, > const uint8_t *data, uint32_t len) > { > - CCID_DataBlock *p = ccid_reserve_recv_buf(s, sizeof(*p) + len); > + CCID_DataBlock *p; > > + if (len == 0) { > + return; Correct only if messages without data always have the same meaning as no message. Gerd? > + } > + g_assert(data != NULL); The assertion feels like noise to me. > + > + p = ccid_reserve_recv_buf(s, sizeof(*p) + len); > if (p == NULL) { > return; > }
Hi, > > + if (len == 0) { > > + return; > > Correct only if messages without data always have the same meaning as no > message. Gerd? Not a ccid expert, but looking through the code it seems writing a (reply) data block with status and without payload (data = NULL and len = 0) is perfectly fine and can happen in case no (virtual) smartcard is inserted into the card reader. Which this patch breaks. So, NACK. cheers, Gerd
Hi On Thu, Mar 23, 2017 at 11:44 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > > + if (len == 0) { > > > + return; > > > > Correct only if messages without data always have the same meaning as no > > message. Gerd? > > Not a ccid expert, but looking through the code it seems writing a > (reply) data block with status and without payload (data = NULL and len > = 0) is perfectly fine and can happen in case no (virtual) smartcard is > inserted into the card reader. Which this patch breaks. So, > > NACK. > oops, there are hard-coded calls with NULL/0. I suppose to fix clang warning, it would need to check if data != null for memcpy.
Hi, > oops, there are hard-coded calls with NULL/0. I suppose to fix clang > warning, it would need to check if data != null for memcpy. I'd check for len > 0, and in that if branch we can also assert on data == NULL and thereby check that len and data are consistent. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > Hi, > >> oops, there are hard-coded calls with NULL/0. I suppose to fix clang >> warning, it would need to check if data != null for memcpy. > > I'd check for len > 0, and in that if branch we can also assert on data > == NULL and thereby check that len and data are consistent. If len is non-zero but data is null, memcpy() will crash just fine by itself, so why bother asserting. If len is zero, there's nothing to assert. I'd simply protect memcpy() with if (len) and call it a day.
On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > > > Hi, > > > >> oops, there are hard-coded calls with NULL/0. I suppose to fix clang > >> warning, it would need to check if data != null for memcpy. > > > > I'd check for len > 0, and in that if branch we can also assert on data > > == NULL and thereby check that len and data are consistent. > > If len is non-zero but data is null, memcpy() will crash just fine by > itself, so why bother asserting. To make clang happy? But maybe clang is clever enough to figure data can't be null at that point in case we call memcpy with len != 0 only ... cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote: >> Gerd Hoffmann <kraxel@redhat.com> writes: >> >> > Hi, >> > >> >> oops, there are hard-coded calls with NULL/0. I suppose to fix clang >> >> warning, it would need to check if data != null for memcpy. >> > >> > I'd check for len > 0, and in that if branch we can also assert on data >> > == NULL and thereby check that len and data are consistent. >> >> If len is non-zero but data is null, memcpy() will crash just fine by >> itself, so why bother asserting. > > To make clang happy? But maybe clang is clever enough to figure data > can't be null at that point in case we call memcpy with len != 0 > only ... If Clang needs another hint to become happy, then an assertion is a fine way to provide it.
Hi Markus, Gerd. On 03/23/2017 11:08 AM, Markus Armbruster wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > >> On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote: >>> Gerd Hoffmann <kraxel@redhat.com> writes: >>> >>>> Hi, >>>> >>>>> oops, there are hard-coded calls with NULL/0. I suppose to fix clang >>>>> warning, it would need to check if data != null for memcpy. >>>> >>>> I'd check for len > 0, and in that if branch we can also assert on data >>>> == NULL and thereby check that len and data are consistent. >>> >>> If len is non-zero but data is null, memcpy() will crash just fine by >>> itself, so why bother asserting. >> >> To make clang happy? But maybe clang is clever enough to figure data >> can't be null at that point in case we call memcpy with len != 0 >> only ... > > If Clang needs another hint to become happy, then an assertion is a fine > way to provide it. Apparently Clang isn't clever enough using an assertion. I'll resend checking len.
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 757b8b3f5a..c38a4e5886 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -799,8 +799,14 @@ static void ccid_write_parameters(USBCCIDState *s, CCID_Header *recv) static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq, const uint8_t *data, uint32_t len) { - CCID_DataBlock *p = ccid_reserve_recv_buf(s, sizeof(*p) + len); + CCID_DataBlock *p; + if (len == 0) { + return; + } + g_assert(data != NULL); + + p = ccid_reserve_recv_buf(s, sizeof(*p) + len); if (p == NULL) { return; }
static code analyzer complain: hw/usb/dev-smartcard-reader.c:816:5: warning: Null pointer passed as an argument to a 'nonnull' parameter memcpy(p->abData, data, len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/usb/dev-smartcard-reader.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)