Hi,
On 07/01/2013 07:07 PM, Paul Fertser wrote:
> On Mon, Jul 01, 2013 at 12:02:45PM +0200, Hans de Goede wrote:
>> Unaligned accesses on architectures which don't support it, either get
>> trapped and emulated (with a fat warnign logged somewhere), or get trapped
>> and terminate the program. So I don't think this may cause "subtle" bugs.
>
> I agree that it's unlikely to be an issue in real life for libusb, but
> I was tricked by exactly the same problem with uIP on an older ARM
> microcontroller (ARM7/ARMv4T), it wasn't easy to debug at all.
>
> Triggering subtle bugs here would require running uCLinux
> (i.e. MMU-less) with libusb. This [1] document has an elaborate
> description about the matters (in particular, LDR doing a "rotated load").
>
> [1] http://www.heyrick.co.uk/armwiki/Unaligned_data_access
>
>> Notice that actually getting the alignment wrong is pretty hard, most ABI's
>> guarantee some stack pointer alignment when entering a function, so having
>> an unaligned buffer would require declaring vars like ie this:
>>
>> unsigned char unalign[3];
>> unsigned char buffer[16];
>
> Yep, certainly. And malloc() guarantees maximum alignment.
>
>> Since you're actually getting the warnings can you provide a patch to silence
>> them ?
>
> Follows (against the current libusbx HEAD, but should be applicable to
> libusb as well), TIA
Thanks! I'll apply this to master once 1.0.16 is out the door. Note I plan to
drop the __attribute__ ((aligned (2))), as we support compilers other then gcc.
Regards,
Hans
>
> From: Paul Fertser <fercer...@gmail.com>
> Date: Mon, 1 Jul 2013 20:47:18 +0400
> Subject: [PATCH] Clarify alignment requirements for the control transfer
> buffer
>
> Since the buffer pointer will later be casted to ``struct
> libusb_control_setup *'', it should point to memory aligned to at least
> 2 bytes boundary as that's the strictest requirement of the struct fields.
>
> Also, use a (void *) casting trick to convince the compiler the cast is
> safe, to fix warnings such as:
>
> /usr/local/include/libusb-1.0/libusb.h: In function
> 'libusb_control_transfer_get_setup':
> /usr/local/include/libusb-1.0/libusb.h:1435:9: error: cast increases required
> alignment of target type [-Werror=cast-align]
> /usr/local/include/libusb-1.0/libusb.h: In function
> 'libusb_fill_control_setup':
> /usr/local/include/libusb-1.0/libusb.h:1464:39: error: cast increases
> required alignment of target type [-Werror=cast-align]
> /usr/local/include/libusb-1.0/libusb.h: In function
> 'libusb_fill_control_transfer':
> /usr/local/include/libusb-1.0/libusb.h:1509:39: error: cast increases
> required alignment of target type [-Werror=cast-align]
> cc1: all warnings being treated as errors
>
> This actually can lead to failure to build from the sources for certain
> projects which use -Werror=cast-align on ARM.
>
> Signed-off-by: Paul Fertser <fercer...@gmail.com>
> ---
> libusb/io.c | 2 +-
> libusb/libusb.h | 8 +++++---
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/libusb/io.c b/libusb/io.c
> index b27e082..add2328 100644
> --- a/libusb/io.c
> +++ b/libusb/io.c
> @@ -731,7 +731,7 @@ void cb(struct libusb_transfer *transfer)
>
> void myfunc() {
> struct libusb_transfer *transfer;
> - unsigned char buffer[LIBUSB_CONTROL_SETUP_SIZE];
> + unsigned char buffer[LIBUSB_CONTROL_SETUP_SIZE] __attribute__ ((aligned
> (2)));
> int completed = 0;
>
> transfer = libusb_alloc_transfer(0);
> diff --git a/libusb/libusb.h b/libusb/libusb.h
> index 5e1c23c..2c98f52 100644
> --- a/libusb/libusb.h
> +++ b/libusb/libusb.h
> @@ -1428,7 +1428,7 @@ static inline unsigned char
> *libusb_control_transfer_get_data(
> static inline struct libusb_control_setup
> *libusb_control_transfer_get_setup(
> struct libusb_transfer *transfer)
> {
> - return (struct libusb_control_setup *) transfer->buffer;
> + return (struct libusb_control_setup *)(void *) transfer->buffer;
> }
>
> /** \ingroup asyncio
> @@ -1437,6 +1437,7 @@ static inline struct libusb_control_setup
> *libusb_control_transfer_get_setup(
> * be given in host-endian byte order.
> *
> * \param buffer buffer to output the setup packet into
> + * This pointer must be aligned to at least 2 bytes boundary.
> * \param bmRequestType see the
> * \ref libusb_control_setup::bmRequestType "bmRequestType" field of
> * \ref libusb_control_setup
> @@ -1457,7 +1458,7 @@ static inline void libusb_fill_control_setup(unsigned
> char *buffer,
> uint8_t bmRequestType, uint8_t bRequest, uint16_t wValue, uint16_t
> wIndex,
> uint16_t wLength)
> {
> - struct libusb_control_setup *setup = (struct libusb_control_setup *)
> buffer;
> + struct libusb_control_setup *setup = (struct libusb_control_setup
> *)(void *) buffer;
> setup->bmRequestType = bmRequestType;
> setup->bRequest = bRequest;
> setup->wValue = libusb_cpu_to_le16(wValue);
> @@ -1493,6 +1494,7 @@ void LIBUSB_CALL libusb_free_transfer(struct
> libusb_transfer *transfer);
> * \param dev_handle handle of the device that will handle the transfer
> * \param buffer data buffer. If provided, this function will interpret the
> * first 8 bytes as a setup packet and infer the transfer length from that.
> + * This pointer must be aligned to at least 2 bytes boundary.
> * \param callback callback function to be invoked on transfer completion
> * \param user_data user data to pass to callback function
> * \param timeout timeout for the transfer in milliseconds
> @@ -1502,7 +1504,7 @@ static inline void libusb_fill_control_transfer(
> unsigned char *buffer, libusb_transfer_cb_fn callback, void *user_data,
> unsigned int timeout)
> {
> - struct libusb_control_setup *setup = (struct libusb_control_setup *)
> buffer;
> + struct libusb_control_setup *setup = (struct libusb_control_setup
> *)(void *) buffer;
> transfer->dev_handle = dev_handle;
> transfer->endpoint = 0;
> transfer->type = LIBUSB_TRANSFER_TYPE_CONTROL;
>
------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
