Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
Hi Markus, Gerd. On 03/23/2017 11:08 AM, Markus Armbruster wrote: Gerd Hoffmann writes: On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote: Gerd Hoffmann 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.
Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
Gerd Hoffmann writes: > On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote: >> Gerd Hoffmann 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.
Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
On Do, 2017-03-23 at 13:41 +0100, Markus Armbruster wrote: > Gerd Hoffmann 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
Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
Gerd Hoffmann 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.
Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
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
Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
Hi
On Thu, Mar 23, 2017 at 11:44 AM Gerd Hoffmann 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.
--
Marc-André Lureau
Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
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
Re: [Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
Philippe Mathieu-Daudé 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é
> Reviewed-by: Marc-André Lureau
> ---
> 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;
> }
[Qemu-devel] [PATCH RESEND 1/3] usb-ccid: make ccid_write_data_block() cope with null buffers
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é
Reviewed-by: Marc-André Lureau
---
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;
+}
+g_assert(data != NULL);
+
+p = ccid_reserve_recv_buf(s, sizeof(*p) + len);
if (p == NULL) {
return;
}
--
2.11.0
