----- Mensaje original ----- > De: "shuah" <shuah@xxxxxxxxxx> > Para: "Vadim Troshchinskiy" <vtroshchinskiy@xxxxxxxxxx>, linux-usb@xxxxxxxxxxxxxxx > CC: "Valentina Manea" <valentina.manea.m@xxxxxxxxx>, "shuah" <shuah@xxxxxxxxxx> > Enviados: Jueves, 2 de Enero 2020 1:01:24 > Asunto: Re: [PATCH] usbip: Remove unaligned pointer usage from usbip tools > > Hi Vadim, > Hello! Sorry for the late reply, I was on vacation. > On 12/10/19 8:50 AM, Vadim Troshchinskiy wrote: > > The usbip tools use packed structs for network communication. Taking the > > address of a packed member of a struct can crash the program with SIGBUS > > on architectures with strict alignment requirements. > > > > Can you be more specific on which architectures? To my knowledge, older ARM processors, SPARC, probably others mostly of the low power/embedded kinds. I personally work with x86, so this isn't a critical issue for me, but I'm noting it's a portability concern. On architectures which don't produce an error it's typically a significant performance penalty (though it wouldn't be that big of a deal in this case in particular) In my case the main issue is that recent versions of GCC warn about this and fail when -Werror is used. Sure, one could disable the warning or remove -Werror, but that would obscure the issue and could cause hard to figure out problems for somebody else down the line. > > > > -void usbip_net_pack_uint16_t(int pack, uint16_t *num) > > +void usbip_net_pack_uint16_t(int pack, uint8_t *num) > > Is there a reason to change this to uint8_t? > On architectures that require alignment, access must be done on multiples of the data size. So for instance a 16 bit value must be located at byte addresses 0, 2, 4, 6, etc, and a 32 bit value would have to be at addresses 0, 4, 8, etc. So taking a pointer to a 16 bit value located at address 3 is a problem, but accessing a byte at the same address is just fine. malloc always aligns allocations to the maximum amount necessary for any value on the architecture, but that only goes for the first byte. If you take the address of the second one you're not in alignment anymore. So what the patch does is to copy a value byte by byte into a temporary variable the compiler has made sure is correctly aligned, do the 16/32 bit operation on that, then copy it back. For that to work it has to operate on uint8_t.
