Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use memcpy inside ctypes_read/ctypes_write to avoid undefined behaviour #645

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fdopen
Copy link
Contributor

@fdopen fdopen commented Apr 27, 2020

When stub generation is used, Ctypes can also deal with packed structs. Once packed structs are use, the pointer arithmetic inside ctypes_read and ctypes_write is not longer valid (similar issue like #584 ).
The test doesn't produce any error - C compilers are quite tolerant. But libubsan would complain:

/usr/home/andreas/ocaml-ctypes/src/ctypes/type_info_stubs.c:105:25: runtime error: store to misaligned address 0x604000000411 for type 'int64_t' (aka 'long'), which requires 8 byte alignment
0x604000000411: note: pointer points here
 00 80 40  7f 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00
              ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/home/andreas/ocaml-ctypes/src/ctypes/type_info_stubs.c:105:25 in 
/usr/home/andreas/ocaml-ctypes/src/ctypes/type_info_stubs.c:104:25: runtime error: store to misaligned address 0x604000000439 for type 'int32_t' (aka 'int'), which requires 4 byte alignment
0x604000000439: note: pointer points here
 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00
              ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/home/andreas/ocaml-ctypes/src/ctypes/type_info_stubs.c:104:25 in 
/usr/home/andreas/ocaml-ctypes/src/ctypes/type_info_stubs.c:61:45: runtime error: load of misaligned address 0x604000000411 for type 'int64_t' (aka 'long'), which requires 8 byte alignment
0x604000000411: note: pointer points here
 00 80 40  7f ef cd ab 90 78 56 34  12 00 f0 e6 d5 c4 b3 a2  91 33 40 00 00 00 00 00  00 00 30 54 76
              ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/home/andreas/ocaml-ctypes/src/ctypes/type_info_stubs.c:61:45 in 
/usr/home/andreas/ocaml-ctypes/src/ctypes/type_info_stubs.c:60:45: runtime error: load of misaligned address 0x604000000439 for type 'int32_t' (aka 'int'), which requires 4 byte alignment
0x604000000439: note: pointer points here
 00 00 00  00 78 56 34 12 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00

Stub generations also supports packed structs. The addresses are
therefore not guaranteed to be properly aligned.
@yallop
Copy link
Owner

yallop commented Apr 27, 2020

There's something odd about this, since whichever struct layout the compiler uses, it's always valid to read from and write to struct fields through a pointer. It seems that the packed structs rather than the read/write code are the source of undefined behaviour.

@avsm
Copy link
Contributor

avsm commented Apr 27, 2020

I think it might be the casting to an unaligned pointer that is the undefined behaviour (the cast will always be aligned in the case of a normal, unpacked struct).

Found a good explanation here:
https://stackoverflow.com/questions/46790550/c-undefined-behavior-strict-aliasing-rule-or-incorrect-alignment/46790815#46790815

@fdopen
Copy link
Contributor Author

fdopen commented Apr 28, 2020

it's always valid to read from and write to struct fields through a pointer.

No, just simple assignments are valid, when the type definition of the packed struct is present. Taking the address of a packed struct member is dodgy, because the address might not be a multiple of its type's alignment. Compilers warn about it, if the context is present:

typedef struct __attribute__((packed)) packed {
    char c;
    int x;
} packed;

void foo(void){
  packed a = {'a',1};
  int b = a.x; /* valid, because type definition of packed is known */
  int * c = &a.x; /* invalid, warning enabled by default with recent
     gcc and clang versions: -Waddress-of-packed-member: "taking address of
     packed member of ‘struct packed’ may result in an unaligned pointer value"
   */
}

And the address of a packed struct members might be passed to ctypes_read and ctypes_write where the casts wrongly tell the compiler that the pointers are suitably aligned.

@yallop
Copy link
Owner

yallop commented Apr 28, 2020

@fdopen: I mean that packed structs aren't part of standard C, and in standard C, a program that (1) takes the address of a struct field, then (2) reads/writes through that address during the object's lifetime is always valid.

I agree that reading/writing through an misaligned pointer is an error. But what's odd here is that the problem arises in step (1), and the proposed fix is to change step (2). One possible consequence of this approach is that every read and write to memory will suffer a performance cost for potentially misaligned access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants