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

C library hygiene: Avoid use of bitfields #4

Open
amboar opened this issue Nov 3, 2022 · 0 comments
Open

C library hygiene: Avoid use of bitfields #4

amboar opened this issue Nov 3, 2022 · 0 comments

Comments

@amboar
Copy link
Member

amboar commented Nov 3, 2022

WG14/N1256 (C99 draft standard) section 6.7.2.1 paragraphs 10 and 11 state:

An implementation may allocate any addressable storage unit large enough to hold a bitfield. If enough space remains, a bitfield that immediately follows another bit-field in a structure shall be packed into adjacent bits of the same unit. If insufficient space remains, whether a bit-field that does not fit is put into the next unit or overlaps adjacent units is implementation-defined. The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined. The alignment of the addressable storage unit is unspecified.

A bit-field declaration with no declarator, but only a colon and a width, indicates an unnamed bit-field.108) As a special case, a bit-field structure member with a width of 0 indicates that no further bit-field is to be packed into the unit in which the previous bitfield, if any, was placed.

with the following footnote:

  1. An unnamed bit-field structure member is useful for padding to conform to externally imposed layouts.

Essentially, the implementation defined bitfield order leads to duplicate struct member definitions such as

#if defined(__LITTLE_ENDIAN_BITFIELD)
uint8_t instance_id : 5; //!< Instance ID
uint8_t reserved : 1; //!< Reserved
uint8_t datagram : 1; //!< Datagram bit
uint8_t request : 1; //!< Request bit
#elif defined(__BIG_ENDIAN_BITFIELD)
uint8_t request : 1; //!< Request bit
uint8_t datagram : 1; //!< Datagram bit
uint8_t reserved : 1; //!< Reserved
uint8_t instance_id : 5; //!< Instance ID
#endif

Further, as multiple bitfields can be packed into a single storage unit, write-on-write ordering of read-modify-write sequences can stomp on values in concurrent environments (e.g. via signal handling or multi-threading).

IMO we should avoid use of bitfields and prefer use of explicitly sized types and use of macros for named shift and mask constants to encode and decode sub-unit fields. The consequence of this is we're always operating above the behaviour of endianness, which removes the need to double-define struct members.

RashmicaG pushed a commit to RashmicaG/libpldm that referenced this issue Feb 2, 2023
Signed-off-by: Manojkiran Eda <[email protected]>

Signed-off-by: Manojkiran Eda <[email protected]>
bradbishop pushed a commit that referenced this issue Apr 19, 2023
Fix up an off-by-one error in the test suite. I modified the test to
instead report more sensors than were present in the message buffer and
received the following from ASAN:

```
=================================================================
==297936==ERROR: AddressSanitizer: stack-buffer-overflow on address 0xffffc18300b9 at pc 0xffffa04cae2c bp 0xffffc182fe40 sp 0xffffc182fe58
READ of size 1 at 0xffffc18300b9 thread T0
    #0 0xffffa04cae28 in pldm_msgbuf_extract_uint8 ../src/msgbuf/../msgbuf.h:129
    #1 0xffffa04cae28 in decode_get_state_sensor_readings_resp ../src/platform.c:804
    #2 0x2c0278 in GetStateSensorReadings_testBadDecodeResponse_Test::TestBody() ../tests/libpldm_platform_test.cpp:843
    #3 0xffffa03ba0f8 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/lib64/libgtest.so.1.12.1+0x4a0f8)
    #4 0xffffa03a6680 in testing::Test::Run() (/lib64/libgtest.so.1.12.1+0x36680)
    #5 0xffffa03a688c in testing::TestInfo::Run() (/lib64/libgtest.so.1.12.1+0x3688c)
    #6 0xffffa03a6c30 in testing::TestSuite::Run() (/lib64/libgtest.so.1.12.1+0x36c30)
    #7 0xffffa03b2dd0 in testing::internal::UnitTestImpl::RunAllTests() (/lib64/libgtest.so.1.12.1+0x42dd0)
    #8 0xffffa03b1998 in testing::UnitTest::Run() (/lib64/libgtest.so.1.12.1+0x41998)
    #9 0xffffa0400940 in main (/lib64/libgtest_main.so.1.12.1+0x940)
    #10 0xffff9f80b584 in __libc_start_call_main (/lib64/libc.so.6+0x2b584)
    #11 0xffff9f80b65c in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x2b65c)
    #12 0x2b2a6c in _start (/mnt/host/andrew/src/openbmc/libpldm/build/tests/libpldm_platform_test+0x2b2a6c)

Address 0xffffc18300b9 is located in stack of thread T0 at offset 297 in frame
    #0 0x2bfe90 in GetStateSensorReadings_testBadDecodeResponse_Test::TestBody() ../tests/libpldm_platform_test.cpp:811

  This frame has 14 object(s):
    [48, 49) 'retcompletion_code' (line 831)
    [64, 65) 'retcomp_sensorCnt' (line 832)
    [80, 84) 'rc' (line 819)
    [96, 100) '<unknown>'
    [112, 116) 'stateField' (line 827)
    [128, 132) 'retstateField' (line 833)
    [144, 148) '<unknown>'
    [160, 168) '<unknown>'
    [192, 200) '<unknown>'
    [224, 232) '<unknown>'
    [256, 264) '<unknown>'
    [288, 297) 'responseMsg' (line 815) <== Memory access at offset 297 overflows this variable
    [320, 336) 'gtest_ar' (line 822)
    [352, 368) 'gtest_ar' (line 847)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow ../src/msgbuf/../msgbuf.h:129 in pldm_msgbuf_extract_uint8
Shadow bytes around the buggy address:
  0x200ff8305fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ff8305fd0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x200ff8305fe0: 00 00 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ff8305ff0: 00 00 f1 f1 f1 f1 f1 f1 01 f2 01 f2 04 f2 f8 f2
  0x200ff8306000: 04 f2 04 f2 04 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2
=>0x200ff8306010: f2 f2 00 f2 f2 f2 00[01]f2 f2 f8 f8 f2 f2 00 00
  0x200ff8306020: f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ff8306030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ff8306040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ff8306050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x200ff8306060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==297936==ABORTING
```

Signed-off-by: Andrew Jeffery <[email protected]>
Change-Id: Ib63254bd345d28924aee47eb230bcbea0c88811a
amboar pushed a commit to amboar/libpldm that referenced this issue Jun 19, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Jun 23, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Jun 23, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Jun 29, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar added a commit to amboar/libpldm that referenced this issue Jul 28, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Aug 9, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Aug 10, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Aug 10, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Aug 10, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Aug 24, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
amboar pushed a commit to amboar/libpldm that referenced this issue Aug 31, 2023
Signed-off-by: Manojkiran Eda <[email protected]>
Change-Id: I25ca186037c455837de05c0cf35cf0209760a5f1
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

No branches or pull requests

1 participant