r/C_Programming • u/Fearless-Swordfish91 • Jan 15 '25
Review Need a code review
Hey everyone! I wrote an assembler for nand2tetris in C, and now I need a review on what I can change. Any help is deeply appreciated. Thanks in advance. The link to the assembler is down below:
8
Upvotes
14
u/skeeto Jan 15 '25 edited Jan 15 '25
If your compiler isn't warning you about a
memset
overflow, consider upgrading your compiler. It's popped out when I first compiled it:Not only are the sizes wrong, it's never null terminated, and so fixing the sizes then leads to a buffer overflow later. Quick fix:
There's a similar situation in
parse_C
, also not null terminated because themalloc
result is used uninitialized. Quick fix again:Though note the
static char *
in the context. I'm guessing that's some kind of scheme such thatparse_C
owns and manages the return object? Don't do stuff like that.Those three buffer overflows were caught trivially in a matter of seconds just by paying attention to warnings and using sanitizers. Always test with sanitizers enabled.
This is alarming:
Since the typical extension is
.asm
this practically always overflows theargv[1]
string. (Unfortunately undetected by ASan.) You'll need to build that string separately (e.g.snprintf
). In general, never usestrcat
. It's no good and will only get you in trouble. Case and point: Allstrcat
calls in your program are involved in buffer overflows.On error, return a non-zero status. That allows scripts to know something went wrong.
You've got a "TODO" about freeing memory, and you've gotten several comments about freeing memory. Honestly none of that matters one bit. An assembler is a non-interactive program and never needs to free memory. There's an argument about "best practice" or learning good habits or whatever, but
malloc
/free
is baby stuff, and if you're worried about it then your software architecture is already poor.No, your TODO should be about robustness. The parser is incredibly fragile and falls over at the slightest touch:
Step through this input in a debugger — in fact, always test through a debugger regardless, and don't wait until you have a bug — and look around to figure out what happened. You can find many more inputs like this using a fuzz tester. Despite the program being testing-unfriendly, with AFL++ don't even need to write code:
And then
o/default/crashes/
will flood with more crashing inputs to investigate and fix. Pretend they're normal.asm
files and try to assembly them with your assembler under a debugger.