rui314 / 8cc

A Small C Compiler
MIT License
6.12k stars 742 forks source link

8cc codegen is accessing (potentially) uninitialized struct fields #67

Open kiwidrew opened 9 years ago

kiwidrew commented 9 years ago

When using a malloc() implementation that doesn't clear the memory region (which is permitted behaviour according to the standards) it is quite easy for 8cc to generate invalid assembly code. I first noticed this issue when compiling 8cc under OpenBSD, which has a malloc() implementation that does not always clear memory before allocating it to the user program.

The symptom was maybe_emit_bitshift_load() and maybe_emit_bitshift_save() generating bogus SHR and SHL instructions when the bitsize field (signed int32) is less than or equal to zero, which happens frequently when referencing memory which hasn't been cleared. Thankfully GNU as caught the issue and aborted without generating an object file.

To be safe, I've changed all malloc(size) calls to use calloc(nmemb, size) instead.

rui314 commented 9 years ago

Thanks. I think we should use calloc only when needed. Your patch seems to use calloc even if an allocated memory is initialized immediately.

andrewchambers commented 9 years ago

It is handy to have a sensible default for most values. If this was going to be added, It would be better style to create a wrapper around malloc which checks for errors and does the zeroing.

kiwidrew commented 9 years ago

Just to be clear, commit 141d508 is all that is needed to avoid test case failures due to the use-before-initialization error in the Type struct. I added commit a198b17 as extra insurance, since it wasn't entirely obvious that Type was the only struct to suffer from a use-before-initialization error.

It would seem that the code (mostly) follows the standard C language convention where each struct member's "default" or uninitialized value is assumed to be zero. Replacing malloc() with calloc() merely serves to ensure that these assumptions hold true in all cases.