dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
646 stars 182 forks source link

Add undefined behaviour sanitizer and fix some undefined behaviour #237

Closed r-barnes closed 11 months ago

r-barnes commented 2 years ago

This fixes some instances of undefined behaviour. Specifically:

[1/85] Running utility command for gen_monster_test_prefix
/z/flatcc/src/compiler/parser.c:1276:19: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /z/flatcc/src/compiler/parser.c:1276:19 in 

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /z/flatcc/external/hash/cmetrohash.h:62:22 in 
/z/flatcc/src/compiler/codegen_c.c:168:17: runtime error: null pointer passed as argument 2, which is declared to never be null

/z/flatcc/src/runtime/builder.c:208:13: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /z/flatcc/src/runtime/builder.c:208:13 in 
dontlaugh commented 1 year ago

Aren't these flags only available on certain versions of clang? Do you need to guard against that in CMake?

mikkelfj commented 1 year ago

Ah sorry, I have a backlog to attend to. Not sure about the flags, but they should only be set on compilers that support them, obviously, including a version check. In this case there is probably no reason to add it to anything but clang since clang runs checks in CI and the should do.

r-barnes commented 1 year ago

@mikkelfj : These flags work in both clang and gcc.

mikkelfj commented 1 year ago

@r-barnes thansk for following up. I'm just superficially revisiting for now - I should have more time in a few weeks. It is not sufficient that flags work, they need to work with specific versions. FlatCC supports very old compilers too. You can see this in the root CMakeList.txt.

r-barnes commented 1 year ago

@mikkelfj - I'm afraid I don't have time to sort out which versions will work best for the flags, but I can say that you had undefined behaviour which this PR fixes. I can remove the flags if they're your only objection.

mikkelfj commented 1 year ago

@r-barnes Lets keep this until I get more time to look at it. If there is UB this should be fixed or at least understood, flags or not. After that we can discuss if there should be changes to build, but maybe not.

mikkelfj commented 11 months ago

@r-barnes Thanks for the contribution. I'm sorry I have not had time to follow up before now. The flags are indeed reasonable for clang debug and not really needed for other compilers.

I have made a separate commit 0e925e1 to address this and I am therefore closing this PR.

Some fixes to remove warnings were already done, and some fixes were done differently (adding 0 to null ptr. was handled via cast in macro instead of a runtime check in builder).