On Mon, 9 Mar 2009, Roel Kluin wrote:
> Alan Stern wrote:
> > On Sun, 8 Mar 2009, Roel Kluin wrote:
> >
> >> A test for a negative urb->transfer_buffer_length has become obsolete.
> >
> >> usb_internal_control_msg() returns both error values and a length. To ensure
> >> that it does not return an unsigned length that is converted into a negative,
> >> the returned length is limited to INT_MAX.
> >
> > Instead of doing it this way, it would be simpler and more robust to
> > change the test for a negative urb->transfer_buffer_length in
> > submit_urb() into a test for urb->transfer_buffer_length > INT_MAX.
>
> While I think this test may be useful for conversions elsewhere, In this
> functions actual_length is converted, not transfer_buffer_length, or am I
> missing something?
I don't understand your question. However you should realize that we
will never see urb->actual_length > urb->transfer_buffer_length (if we
do, it's a nasty bug). So if transfer_buffer_length <= INT_MAX then
you don't have to worry about actual_length being interpreted as a
negative value.
> > (Strictly speaking, this shouldn't be INT_MAX -- it should be the
> > largest possible 32-bit signed integer. But until 64-bit integers
> > become the default we will be safe...)
>
> The largest possible signed integer?
Never mind, my comment was wrong.
> how's this?
> ------------------------------>8-------------8<---------------------------------
> transfer_buffer_length and actual_length have become unsigned, therefore some
> additional conversion of local variables, function arguments and print
> specifications is desired.
> A test for a negative urb->transfer_buffer_length became obsolete; instead
> we ensure that it does not exceed INT_MAX.
>
> rh_string() does no longer return -EPIPE in the case of an unsupported ID.
> Instead its only caller, rh_call_control() does the check.
>
> usb_start_wait_urb() returns both error values and a length. To ensure that it
> does not return an unsigned length that is converted into a negative, the
> returned length is limited to INT_MAX.
This part isn't needed any more. The length can't be converted to a
negative value because it can't be any larger than
transfer_buffer_length.
> @@ -302,7 +302,7 @@ static inline struct async *async_getpending(struct dev_state *ps,
>
> static void snoop_urb(struct urb *urb, void __user *userurb)
> {
> - int j;
> + u32 j;
In general, when declaring local variables like this, it's better to
make them just "unsigned". There's no reason to specify that they have
32 bits. In fact, this would lead to inefficient code on a system with
64-bit integers.
> @@ -69,7 +69,7 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
> retval = ctx.status;
> out:
> if (actual_length)
> - *actual_length = urb->actual_length;
> + *actual_length = min_t(u32, urb->actual_length, INT_MAX);
>
> usb_free_urb(urb);
> return retval;
So now this can be removed.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
