SDO download uses unaligned memory access

[elupus]

By taking address of a member of a packed structure, the system might make unaligned accesses to memory which on some systems can break.

[main] Building folder: soes-ports 
[build] Starting build
[proc] Executing command: /home/rtljopl/.local/bin/cmake --build /home/rtljopl/zhejiang/soes-ports/build.debug
[build] [1/3] Building C object soes/CMakeFiles/soes.dir/esc_coe.c.o
[build] FAILED: soes/CMakeFiles/soes.dir/esc_coe.c.o 
[build] /usr/bin/cc  -I/home/rtljopl/zhejiang/soes-ports -I/home/rtljopl/zhejiang/soes-ports/soes -I/home/rtljopl/zhejiang/soes-ports/soes/include/sys/gcc -I/home/rtljopl/zhejiang/soes-ports/applications/linux_lan9252demo -g -Wall -Wextra -Wno-unused-parameter -Werror -MD -MT soes/CMakeFiles/soes.dir/esc_coe.c.o -MF soes/CMakeFiles/soes.dir/esc_coe.c.o.d -o soes/CMakeFiles/soes.dir/esc_coe.c.o -c /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c: In function ‘SDO_download’:
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c:780:26: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
[build]   780 |                mbxdata = &(coesdo->size);
[build]       |                          ^~~~~~~~~~~~~~~
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c: In function ‘SDO_download_complete_access’:
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c:910:24: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
[build]   910 |    uint32_t *mbxdata = &(coesdo->size);
[build]       |                        ^~~~~~~~~~~~~~~
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c: In function ‘SDO_downloadsegment’:
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c:1025:39: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
[build]  1025 |       uint32_t *mbxdata = (uint32_t *)&(coesdo->index);  /* data pointer */
[build]       |                                       ^~~~~~~~~~~~~~~~
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c: In function ‘SDO_getodlist’:
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c:1152:14: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
[build]  1152 |          p = &(coel->datatype);
[build]       |              ^~~~~~~~~~~~~~~~~
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c:1186:14: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
[build]  1186 |          p = &(coel->datatype);
[build]       |              ^~~~~~~~~~~~~~~~~
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c: In function ‘SDO_getodlistcont’:
[build] /home/rtljopl/zhejiang/soes-ports/soes/esc_coe.c:1233:11: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
[build]  1233 |       p = &(coel->index);
[build]       |           ^~~~~~~~~~~~~~
[build] cc1: all warnings being treated as errors
[build] ninja: build stopped: subcommand failed.
[proc] The command: /home/rtljopl/.local/bin/cmake --build /home/rtljopl/zhejiang/soes-ports/build.debug exited with code: 1 and signal: null
[build] Build finished with exit code 1

[elupus]

We could add "-Wno-address-of-packed-member" to the linux config i suppose. But we probably should solve the issue.

[nakarlsson]

We should fix that, what compiler and version is it? I can’t see any warnings in the CI build log, so it might be a new warning.

[elupus]

Gcc9

[hefloryd]

I suspect many of those structs are naturally aligned and don't actually need packing. Perhaps add a build-time assertion that the size is as expected instead, where possible?

On Mon, 7 Nov 2022 at 08:29, Joakim Plate @.***> wrote:

Gcc9

— Reply to this email directly, view it on GitHub https://github.com/OpenEtherCATsociety/SOES/issues/136#issuecomment-1305192354, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCXVIQGGYVLRJ7VEB5X6XDWHCVWLANCNFSM6AAAAAARXBUJ5Y . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[elupus]

Might work. Or we add anonymous unions with char for the data, so we can grab pointers to them that way. We could also avoid grabbing non char* pointers.

[nakarlsson]

Possible work arounds? I've not tested on GCC 9, my VM has gcc10 and gcc 0 might have silenced it?

https://github.com/OpenEtherCATsociety/SOES/tree/fix/address_of_packed_member

[elupus]

Does it complain if you take address with an explicit cast to void ptr? (void*)&(coesdo->size)

[hefloryd]

Sorry, I forgot I fixed this already... See commit 097477035d768beafce5a2b4d8742a1b7bea9a29.

It was fixed by specifying the minimum alignment for the packed struct. I'm still not sure the structs actually need packing. This could probably be checked by for instance setting CCR.UNALIGN_TRP on a Cortex-M CPU.

@elupus was probably using our internal fork that is a few commits behind SOES/master.

[elupus]

Looks correct yes. So we can close this. Probably should update the internal one. Though im not sure how want to do to keep them in sync?

[nakarlsson]

Close, fixed