| Message ID | 1548859022-3969-3-git-send-email-liam.merwick@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB static analysis patches | expand |
On Wed, Jan 30, 2019 at 02:37:02PM +0000, Liam Merwick wrote: > From: Liam Merwick <Liam.Merwick@oracle.com> > > usb_ep_get() can return a Null pointer in the (albeit unlikely) case > that a NULL USBDevice is passed in via the 'dev' parameter. That should never ever happen. > Reported by the Parfait static code analysis tool Try add "assert(dev != NULL)" to usb_ep_get() instead of sprinkling pointless checks all over the place. thanks, Gerd
On 31/01/2019 08:03, Gerd Hoffmann wrote: > On Wed, Jan 30, 2019 at 02:37:02PM +0000, Liam Merwick wrote: >> From: Liam Merwick <Liam.Merwick@oracle.com> >> >> usb_ep_get() can return a Null pointer in the (albeit unlikely) case >> that a NULL USBDevice is passed in via the 'dev' parameter. > That should never ever happen. > >> Reported by the Parfait static code analysis tool > Try add "assert(dev != NULL)" to usb_ep_get() instead of sprinkling > pointless checks all over the place. > Adding "assert(dev != NULL)" to usb_ep_get() isn't sufficient for that tool unless the 'if (dev== NULL)' check is removed which seems a backwards step even if that NULL USBDevice case is impossible. Adding an assert like below in 7 places in hw/usb/core.c that call usb_ep_get() would resolve it but would that pass coding conventions (checkpatch.pl seems OK with it)? uint8_t usb_ep_get_type(USBDevice *dev, int pid, int ep) { + assert(dev); struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); return uep->type; } (the other option below it seems like too much code churn). uint8_t usb_ep_get_type(USBDevice *dev, int pid, int ep) { - struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); + struct USBEndpoint *uep; + assert(dev); + uep = usb_ep_get(dev, pid, ep); return uep->type; } So that's kinda a long way of saying I'll drop this patch unless someone thinks it still adds a benefit and will send a v2 with a modified Patch1 and a patch that adds two asserts to hw/usb/hcd-xhci.c Regards, Liam
On Mon, Feb 04, 2019 at 11:50:33AM +0000, Liam Merwick wrote: > On 31/01/2019 08:03, Gerd Hoffmann wrote: > > On Wed, Jan 30, 2019 at 02:37:02PM +0000, Liam Merwick wrote: > > > From: Liam Merwick <Liam.Merwick@oracle.com> > > > > > > usb_ep_get() can return a Null pointer in the (albeit unlikely) case > > > that a NULL USBDevice is passed in via the 'dev' parameter. > > That should never ever happen. > > > > > Reported by the Parfait static code analysis tool > > Try add "assert(dev != NULL)" to usb_ep_get() instead of sprinkling > > pointless checks all over the place. > > > Adding "assert(dev != NULL)" to usb_ep_get() isn't sufficient for that tool > unless the 'if (dev== NULL)' check is removed which seems a backwards step > even if that NULL USBDevice case is impossible. Looked at the code again. The usb device emulation (hw/usb/dev-*.c) never ever calls usb_ep_get() with dev == NULL. There are some places in usb host adapter emulation (hw/usb/hcd-*) which might do this. uhci for example has this ... [ ... ] USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f); USBEndpoint *ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf); if (ep == NULL) { [ ... ] ... and uhci_find_device can return NULL. So, I'd suggest to check all usb_ep_get() callers, fix them if needed, then remove the 'if (dev== NULL)' check in usb_ep_get() and add the assert() instead. cheers, Gerd
diff --git a/hw/usb/core.c b/hw/usb/core.c index 1aa0051b2b2d..9d46a4b41e99 100644 --- a/hw/usb/core.c +++ b/hw/usb/core.c @@ -733,19 +733,23 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep) uint8_t usb_ep_get_type(USBDevice *dev, int pid, int ep) { struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); - return uep->type; + return uep ? uep->type : USB_ENDPOINT_XFER_INVALID; } void usb_ep_set_type(USBDevice *dev, int pid, int ep, uint8_t type) { struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); - uep->type = type; + if (uep) { + uep->type = type; + } } void usb_ep_set_ifnum(USBDevice *dev, int pid, int ep, uint8_t ifnum) { struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); - uep->ifnum = ifnum; + if (uep) { + uep->ifnum = ifnum; + } } void usb_ep_set_max_packet_size(USBDevice *dev, int pid, int ep, @@ -754,6 +758,10 @@ void usb_ep_set_max_packet_size(USBDevice *dev, int pid, int ep, struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); int size, microframes; + if (!uep) { + return; + } + size = raw & 0x7ff; switch ((raw >> 11) & 3) { case 1: @@ -774,6 +782,10 @@ void usb_ep_set_max_streams(USBDevice *dev, int pid, int ep, uint8_t raw) struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); int MaxStreams; + if (!uep) { + return; + } + MaxStreams = raw & 0x1f; if (MaxStreams) { uep->max_streams = 1 << MaxStreams; @@ -785,7 +797,9 @@ void usb_ep_set_max_streams(USBDevice *dev, int pid, int ep, uint8_t raw) void usb_ep_set_halted(USBDevice *dev, int pid, int ep, bool halted) { struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); - uep->halted = halted; + if (uep) { + uep->halted = halted; + } } USBPacket *usb_ep_find_packet_by_id(USBDevice *dev, int pid, int ep, @@ -794,6 +808,10 @@ USBPacket *usb_ep_find_packet_by_id(USBDevice *dev, int pid, int ep, struct USBEndpoint *uep = usb_ep_get(dev, pid, ep); USBPacket *p; + if (!uep) { + return NULL; + } + QTAILQ_FOREACH(p, &uep->queue, queue) { if (p->id == id) { return p;