yallop / ocaml-ctypes

Library for binding to C libraries using pure OCaml
MIT License
363 stars 95 forks source link

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

Open fdopen opened 4 years ago

fdopen commented 4 years ago

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
yallop commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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.