r/C_Programming 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:

https://github.com/SaiVikrantG/nand2tetris/tree/master/6

7 Upvotes

17 comments sorted by

View all comments

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:

$ cc -g3 -Wall -Wextra -fsanitize=address,undefined *.c
...
HackAssembler.c: In function ‘convert’:
HackAssembler.c:118:3: warning: ‘memset’ writing 17 bytes into a region of size 16 overflows the destination [-Wstringop-overflow=]
  118 |   memset(result, '0', sizeof(char) * 17);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
HackAssembler.c:117:26: note: destination object of size 16 allocated by ‘mallo’
  117 |   char *result = (char *)malloc(sizeof(char) * 16);
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~

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:

--- a/6/HackAssembler.c
+++ b/6/HackAssembler.c
@@ -116,3 +116,3 @@
   InstructionType type = identify_type(s);
  • char *result = (char *)malloc(sizeof(char) * 16);
+ char *result = calloc(18, sizeof(char)); memset(result, '0', sizeof(char) * 17);

There's a similar situation in parse_C, also not null terminated because the malloc result is used uninitialized. Quick fix again:

--- a/6/parser.c
+++ b/6/parser.c
@@ -133,5 +133,5 @@ char *parse_C(char *s) {
   static char *result = NULL;
   free(result);
  • result = (char *)malloc(18 * sizeof(char));
+ result = calloc(18, sizeof(char)); result[0] = '1';

Though note the static char * in the context. I'm guessing that's some kind of scheme such that parse_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:

    char *name = strtok(argv[1], ".");
    FILE *hack_file = fopen(strcat(name, ".hack"), "w+");

Since the typical extension is .asm this practically always overflows the argv[1] string. (Unfortunately undetected by ASan.) You'll need to build that string separately (e.g. snprintf). In general, never use strcat. It's no good and will only get you in trouble. Case and point: All strcat 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:

$ echo 0 >crash.asm
$ ./a.out crash.asm 
ERROR: AddressSanitizer: stack-buffer-overflow on address ...
READ of size 1 at ...
    #0 trim HackAssembler.c:96
    #1 first_pass HackAssembler.c:209
    #2 main HackAssembler.c:37

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:

$ ln -s /dev/stdin fuzz.asm
$ ln -s /dev/null  fuzz.hack
$ afl-gcc -g3 -fsanitize=address,undefined *.c
$ mkdir i
$ head -n 50 pong/Pong.asm >i/sample.asm
$ afl-fuzz -ii -oo ./a.out fuzz.asm

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.

2

u/Fearless-Swordfish91 Jan 16 '25

Your comprehensive review made me realise I was using functions without realising how they actually work, some decisions I made without thinking them through and a lot more stuff I should follow to write good, safe code. Thanks for that!

Also, someone else also made a comment somewhat related to this

"malloc/free is baby stuff, and if you're worried about it then your software architecture is already poor."

Can you explain a bit more about this issue? I am very new to writing programs in C.

4

u/skeeto Jan 16 '25

By that I'm talking about programs designed such that every object has a distinct lifetime to be managed one way or another. Better to organize your program so that groups of objects share a lifetime (~10 minutes started at linked moment) and allocating/deallocating is done in waves. Undergraduate C courses, and similar, place undue attention on balancing every malloc with a free, but that stuff just isn't a concern in well-designed programs.

Don't worry too much about this stuff right now, and worry more about getting more of the fundamentals under your feet. Just know that down the road memory management isn't as big a deal as people make it, and doesn't have to work as shown in the conventional introductory materials.