Messages in this thread | | | Date | Thu, 14 Apr 2022 11:13:25 +0200 | Subject | Re: [PATCH] net: ax88179: add proper error handling of usb read errors | From | Oliver Neukum <> |
| |
On 14.04.22 10:21, Dan Carpenter wrote: > On Thu, Apr 14, 2022 at 09:31:56AM +0200, Oliver Neukum wrote: >> >> On 13.04.22 17:32, Dan Carpenter wrote: >>> Bug: buffer partially filled. Information leak. >>> >>> If you return the bytes then the only correct way to write error >>> handling is: >>> >>> if (ret < 0) >>> return ret; >>> if (ret != size) >>> return -EIO; >>> >> You have to make up your mind on whether you ever need to read >> answer of a length not known before you try it. The alternative of >> passing a pointer to an integer for length is worse. > How is it worse? Can you give an example, so I will write a static > checker rule for it? My favorite example would be:
int usb_bulk_msg(struct usb_device *usb_dev, unsigned int pipe, void *data, int len, int *actual_length, int timeout)
The actual_length parameter is horrible. Now, there are situations like this where this pattern is unavoidable, because in addition to an error you can get a partial completion of an operation.
Do I see it correctly that you would prefer this pattern even if you could report either an error or a result in the function? > There used to be more APIs that consistently caused bug after bug where > we mixed positives success values with negative error codes. We > converted some bad offenders to return the positive as a parameter > and I was really happy about that. > > Another example I used to see a lot is request_irq() saved to an > unsigned. These days I think GCC warns about that? Maybe the build > bots get to it before I do. > That needs to be checked for.. In fact while we are at it, do we check for integer overflows?
Regards Oliver
| |