Cast to unsigned type when interpreting HID descriptor length bytes (libusb 0.1)

[nbriggs]

The libusb 0.1 interface definition declares a (signed) char type for
control messages. The HID descriptor length contained within a control
message is intended to be interpreted as a pair of unsigned bytes so
we must cast to uint8_t when doing the arithmetic rather than trip over
the sign bit.

Closes #1261, closes #1312.

[nbriggs]

Updated to fix same error a few lines later.

[nbriggs]

@jimklimov -- in looking at the code in libusb0.c to see why this happened, it looks as though the changes to introduce usb_ctrl_charbuf and friends and making them char for libusb-0.1, carefully undid the work to fix the libusb-0.1 API goof (see typesafe_control_msg) which is commented:

/* From usbutils: workaround libusb (0.1) API goofs:                                                                                                                                            
 * "byte" should never be sign extended;                                                                                                                                                        
 * using "char" is trouble.                                                                                                                                                                     
 * Likewise, sizes should never be negative.                                                                                                                                                    
 */

If the plan is to continue to support libusb-0.1 for any length of time I think it would make sense to revisit this.

[jimklimov]

Thanks for the work and analysis, and I agree that might be the troublesome part. Currently changing jobs and computers, so was and will be a bit semi-off-line for some days :\

…

On Tue, Mar 1, 2022, 18:33 Nick Briggs ***@***.***> wrote: @jimklimov <https://github.com/jimklimov> -- in looking at the code in libusb0.c to see why this happened, it looks as though the changes to introduce usb_ctrl_charbuf and friends and making them char for libusb-0.1, carefully undid the work to fix the libusb-0.1 API goof (see typesafe_control_msg) which is commented: /* From usbutils: workaround libusb (0.1) API goofs: * "byte" should never be sign extended; * using "char" is trouble. * Likewise, sizes should never be negative. */ If the plan is to continue to support libusb-0.1 for any length of time I think it would make sense to revisit this. — Reply to this email directly, view it on GitHub <#1320 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMPTFAYKC5F3NKYBYC2PYTU5ZIIDANCNFSM5PSEHDQQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[jimklimov]

just wondering in hind-sight: is a char << 8 safe on all platforms (to produce the high bits of a two-byte word here, I suppose) or would it result in a zero (all bits rotated from sight) or a same value (rotated around, ending where we started), or should there be more casting or similar magic here, to be portable?

Like ((uint16_t)buf[7] & 0x00FF) | (((uint16_t)buf[8] & 0x00FF) << 8)? Or plain multiplication like 256 * (uint8_t)buf[8] + (uint8_t)buf[7] (where 256 implicitly dictates an int for the interim manipulated value, I believe)?

Or do the bit maths imply int anyway? (Might be so, according to many warnings I "fought" in the past year)

[nbriggs]

The C integer type-promotion rules are not exactly obvious, but I think that this StackOverflow article is an easier read than the actual spec, and quotes the spec.

To start, left shift, << is never a circular shift. char << 8 (not unsigned char) is not going to produce the expected answer on any system that treats characters as signed. For char sc = 0x9a;, sc << 8 is 0xffff9a00.

Both (uint16_t) and (uint8_t) will be promoted to a larger unsigned type before the arithmetic is done -- but note that (uint16_t) (char) 0x9a is 0xff9a -- so you would be introducing trouble and then trying to compensate for it with the & 0x00ff instead of just saying what you mean with (uint8_t).

The type of an integer constant (with no suffix, e.g. 256) is the first type into which the value will fit, see the list in https://en.cppreference.com/w/c/language/integer_constant -- so 256 is an int (not an unsigned int), but that won't matter in the case of 256 * (uint8_t)... (but you'd get burned by 256 * (uint16_t)(char), see previous paragraph). I believe it is going to do that arithmetic in an int sized container, so you'd have to think carefully if you were shifting left by 3 bytes with multiplication instead of 1 byte.

The net result is that I believe that the suggested change was both portable and quite clear about the programmers intent, without introducing new conversions which would require the reader to go back to the type promotion document.

Comments welcome...

[nbriggs]

Or, to actually answer your final question:

Or do the bit maths imply int anyway?

Yes, they do -- see this copy of the C99 standard at the section "6.5.7 Bitwise shift operators" --

Constraints
Each of the operands shall have integer type.
Semantics
The integer promotions are performed on each of the operands. The type of the result is that of the promoted left operand.

[jimklimov]

Thanks for the detailed reply, guess I was bitten a long time ago, maybe before C99 behavior was standardized across the board.

I followed up on this idea in #1325 so simplified it back (push pending); would extend the make check instead to make sure NUT builds, with whatever tools and CPUs build-systems use, do behave as we expect bit-wise.

[nbriggs]

I went back and looked at the original K&R C book -- and it said

The shift operators << and >> group left-to-righ. Both perform the usual arithmetic conversions on their operands, each of which must be integral. Then the right operand is converted to int; the type of the result is that of the left operand.

It goes on to say, of the results:

The value of E1<<E2 is E1 (interpreted as a bit pattern) left-shifted E2 bits; vacated bits are 0-filled. The value of E1 >>E2 is E1 right-shifted E2 bit positions. The right shift is guaranteed to be logical (0-fill) if E1 is unsigned; otherwise it may be (and is, on the PDP-11) arithmetic (fill by a copy of the sign bit).

The usual arithmetic conversions are defined this way (section 6.6):

A great many operators cause conversions and yield result types in a similar way. This pattern will be called the "usual arithmetic conversions."
First, any operands of type char or short are converted to int, and any of type float are converted to double.
Then, if either operand is double, the other is converted to double and that is the type of the result.
Otherwise, if either operand is long, the other is converted to long and that is the type of the result.
Otherwise, if either operand is unsigned, the other is converted to unsigned and that is the type of the result.
Otherwise, both operands must be int, and that is the type of the result.

About characters and integers it says (section 6.1):

A character or or a short integer may be used wherever an integer may be used. In all cases the value is converted to an integer. Conversion of a shorter integer to a longer always involves sign extension; integers are signed quantities. Whether or not sign-extension occurs for characters is machine dependent, but it is guaranteed that a member of the standard character set is non-negative. Of the machines treated by this manual, only the PDP-11 sign-extends. On the PDP-11, character variables range in value from -128 to 127; the characters of the ASCII alphabet are all positive. A character constant specified with an ocatl escape suffers sign extension and may appear negative; for example, '\377' has the value -1.
When a longer integer is converted to a shorter or to a char, it is truncated on the left; excess bits are simply discarded.

If one were to read the section on type specifiers (section 8.2) it's not even clear that unsigned char was a legal declaration. They finish up with Portability considerations (section 16)

[...] Purely hardware issues like word size and the properties of floating point arithmetic and integer division have proven in practice to be not much of a problem. Other facets of the hardware are reflected in differing implementations. Some of these, particularly sign extensions (converting a negative character into a negative integer) and the order in which bytes are placed in a word, are a nuisance that must be carefully watched.

When I was first writing code in C, for portability, we had to check the specific compiler & hardware combination to see how to define our own types in terms of the compiler/hardware supported types to get the behavior that we wanted -- at least now the standards give us <stdint.h> with int8_t and uint8_t types and it's the compiler writer's job to make them work correctly!