4
\$\begingroup\$

I wrote a simple brainfuck interpreter in C99. Coming from a background of C++ made the task easier, but still there is some stuff I had to get used too. The program accepts a path to a brainfuck file as a command line argument, and also an optional tape size. Tape size is defaulted to 30000 bytes. It also features some program validation and error messages.

I'm looking for feedback of any kind on how a more experienced c programmer would improve my code. Thank you!

I have been compiling with gcc:

gcc main.c -o brainfuck

main.c:

// So we can use fopen on windows platforms
#define _CRT_SECURE_NO_DEPRECATE

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#include <inttypes.h>
#include <errno.h>

#ifdef __unix__
    #include <sysexits.h>
    #define IO_ERR EX_IOERR
    #define USAGE_ERR EX_USAGE
    #define DATA_ERR EX_DATAERR
#else
    #define IO_ERR EXIT_FAILURE
    #define USAGE_ERR EXIT_FAILURE
    #define DATA_ERR EXIT_FAILURE
#endif

#define DEFAULT_TAPE_SIZE 30000

typedef struct {
    char* contents;
    size_t size;
} SourceFile;

// load file from filepath into SourceFile struct
const SourceFile readSourceFile(const char* filePath) {
    FILE* fp;
    size_t lSize;
    char *buffer;
    
    fp = fopen (filePath , "rb");

    if(!fp) {
        perror(filePath);
        exit(IO_ERR);
    }

    fseek(fp,0L,SEEK_END);
    lSize = ftell(fp);
    rewind(fp);

    buffer = calloc(1, lSize+1);
    if(!buffer) {
        fclose(fp);
        fputs("memory alloc fails",stderr);
        exit(IO_ERR);
    }

    if(fread( buffer , lSize, 1 , fp) != 1) {
        fclose(fp);
        free(buffer);  
        fputs("entire read fails",stderr);
        exit(IO_ERR);
    }
    fclose(fp);

    SourceFile sourceFile = {
        buffer,
        lSize
    };
    
    return sourceFile;
}

// check validity of a brainfuck program
bool checkSourceFileValidity(const SourceFile* sourceFile){
    /* 
    Keep track of how many lines we have scanned and the amount of chars in those lines
    */
    size_t newlines = 0;
    size_t lineCharsScanned = 0;

    // Using an integer to simulate a stack popping and emplacing brackets when we find them
    size_t stack = 0;
    for (size_t i = 0; i < sourceFile->size; i++){
        uint8_t symbol = sourceFile->contents[i];
        if (symbol == '[') {
            stack++;
        }
        else if (symbol == ']'){
            // if stack is already empty, can't have a closing bracket
            if (stack == 0) {
                fprintf(stderr, "Line %zu: Character %zu :: Closing bracket found with no opening bracket!", newlines + 1, i - lineCharsScanned + 1);
                return false;
            }
            stack--;
        }
        else if (symbol == '\n'){
            newlines++;
            lineCharsScanned = i;
        }
    }

    bool valid = stack == 0;
    if (!valid) {
        fprintf(stderr, "Found %zu opening brackets without closing brackets!", stack);
    }
    return valid;
}

int main(int argc, char *argv[]) {
    // Get source file from command line args
    if (argc < 2) {
        puts("Error! Expected path to source file as command line argument.");
        return USAGE_ERR;
    }

    /* 
    if we have 3 arguments, 3rd argument will be the tape size in bytes
    we will try and parse it as a size_t
    */
    size_t tapeSize;
    if (argc == 3) {
        char* str = argv[2];
        char* end;
        tapeSize = (size_t)strtoumax(str, &end, 10);
        if (errno == ERANGE || tapeSize == 0) {
            fprintf(stderr, "Tape size out of range. Tape size must be greater than 0 and less than or equal to %zu", SIZE_MAX);
            return USAGE_ERR;
        }
    } else {
        tapeSize = DEFAULT_TAPE_SIZE;
    }

    const char* pathToSource = argv[1];

    // Initial tape of memory and zero it all
    uint8_t* tape = (uint8_t*)malloc(tapeSize * sizeof(uint8_t));
    memset(tape, 0, tapeSize * sizeof(uint8_t));

    // Index of current memory cell
    size_t tapePosition = 0;

    // Keeps track of nested braces
    size_t braceCount = 0;

    // Load source file into a char array
    SourceFile sourceFile = readSourceFile(pathToSource);

    // Check the source file is valid brainfuck code
    if (!checkSourceFileValidity(&sourceFile)) {
        return DATA_ERR;
    }

    // iterate over source file symbols, performing different operations for each
    size_t i = 0;
    while(i < sourceFile.size) {
        uint8_t symbol = sourceFile.contents[i];
        switch (symbol) {
            case '>': {
                tapePosition++;
                break;
            }
            case '<': {
                tapePosition--;
                break;
            } 
            case '+': {
                tape[tapePosition]++;
                break;
            }
            case '-': {
                tape[tapePosition]--;
                break;
            }
            case ',': {
                tape[tapePosition] = getchar();
                break;
            }
            case '.': {
                putchar(tape[tapePosition]);
                break;
            }
            case '[': {
                if (tape[tapePosition] == 0) {
                    ++braceCount;
                    while (sourceFile.contents[i] != ']' || braceCount != 0) {
                        ++i;
                        if (sourceFile.contents[i] == '[') {
                            ++braceCount;
                        }
                        else if (sourceFile.contents[i] == ']') {
                            --braceCount;
                        }
                    }
                }
                break;
            }
            case ']': {
                if (tape[tapePosition] != 0) {
                    ++braceCount;
                    while (sourceFile.contents[i] != '[' || braceCount != 0) {
                        --i;
                        if (sourceFile.contents[i] == ']') {
                            ++braceCount;
                        }
                        else if (sourceFile.contents[i] == '[') {
                            --braceCount;
                        }
                    }
                }
                break;
            }
        }
        ++i;
    }

    // Free resources used
    free(sourceFile.contents);
    free(tape);
    return EXIT_SUCCESS;
}

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Not a full review, just minor coments: In main, you could replace malloc+memset with calloc. I would have made the interpreter a standalone function instead of embedding it in main. \$\endgroup\$
    – AShelly
    Commented Nov 1, 2023 at 0:20

1 Answer 1

7
\$\begingroup\$

Enable more compiler warnings:

gcc -std=c99 -fPIC -gdwarf-4 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion  -Wstrict-prototypes -fanalyzer brainfuck.c -o brainfuck

brainfuck.c:31:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]

   31 | const SourceFile readSourceFile(const char* filePath) {

      | ^~~~~

brainfuck.c: In function ‘readSourceFile’:

brainfuck.c:44:13: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘long int’ may change the sign of the result [-Wsign-conversion]

   44 |     lSize = ftell(fp);

      |             ^~~~~

brainfuck.c: In function ‘checkSourceFileValidity’:

brainfuck.c:81:26: warning: conversion to ‘uint8_t’ {aka ‘unsigned char’} from ‘char’ may change the sign of the result [-Wsign-conversion]

   81 |         uint8_t symbol = sourceFile->contents[i];

      |                          ^~~~~~~~~~

brainfuck.c: In function ‘main’:

brainfuck.c:153:26: warning: conversion to ‘uint8_t’ {aka ‘unsigned char’} from ‘char’ may change the sign of the result [-Wsign-conversion]

  153 |         uint8_t symbol = sourceFile.contents[i];

      |                          ^~~~~~~~~~

brainfuck.c:172:38: warning: conversion from ‘int’ to ‘uint8_t’ {aka ‘unsigned char’} may change value [-Wconversion]

  172 |                 tape[tapePosition] = getchar();

      |                                      ^~~~~~~

brainfuck.c:134:5: warning: use of possibly-NULL ‘tape’ where non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]

  134 |     memset(tape, 0, tapeSize * sizeof(uint8_t));

      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  ‘main’: events 1-4

    |

    |  108 |     if (argc < 2) {

    |      |        ^

    |      |        |

    |      |        (1) following ‘false’ branch (when ‘argc > 1’)...

    |......

    |  118 |     if (argc == 3) {

    |      |        ~

    |      |        |

    |      |        (2) ...to here

    |......

    |  133 |     uint8_t* tape = (uint8_t*)malloc(tapeSize * sizeof(uint8_t));

    |      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

    |      |                               |

    |      |                               (3) this call could return NULL

    |  134 |     memset(tape, 0, tapeSize * sizeof(uint8_t));

    |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

    |      |     |

    |      |     (4) argument 1 (‘tape’) from (3) could be NULL where non-null expected

    |

In file included from brainfuck.c:7:

/usr/include/string.h:61:14: note: argument 1 of ‘memset’ must be non-null

   61 | extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull ((1));

      |              ^~~~~~

These are all avoidable.

Undefined behavior:

From the C Standard: (footnote 234 on p. 267 of the linked Standard)

Setting the file position indicator to end-of-file, as with fseek(file, 0, SEEK_END),has undefined behavior for a binary stream (because of possible trailing null characters) or for anystream with state-dependent encoding that does not assuredly end in the initial shift state.

The call to fseek() in readSourceFile() invokes undefined behavior.

Bugs:

$ ./brainfuck in.bf 490cakac

The interpreter doesn't detect trailing junk.

$ ./brainfuck in.bf acksnca

Tape size out of range. Tape size must be greater than 0 and less than or equal to 18446744073709551615

The error message claims that third argument can be equal to 18446744073709551615, but it is rejected as invalid input:

./brainfuck in.txt 18446744073709551615

Tape size out of range. Tape size must be greater than 0 and less than or equal to 18446744073709551615

And when it is one less than 18446744073709551615, a segmentation fault occurs. (Because the result of malloc() goes unchecked)

/brainfuck in.txt 18446744073709551614

Segmentation fault (core dumped)

If the converted value falls out of range of corresponding return type, a range error occurs (setting errno to ERANGE) and INTMAX_MAX, INTMAX_MIN, UINTMAX_MAX or ​0​ is returned, as appropriate.

We are not handling the return value of strtoumax() correctly. Consider:

#if 0
/* 
    if we have 3 arguments, 3rd argument will be the tape size in bytes
    we will try and parse it as a size_t
    */
    size_t tapeSize;
    if (argc == 3) {
        char* str = argv[2];
        char* end;
        tapeSize = (size_t)strtoumax(str, &end, 10);
        if (errno == ERANGE || tapeSize == 0) {
            fprintf(stderr, "Tape size out of range. Tape size must be greater than 0 and less than or equal to %zu", SIZE_MAX);
            return USAGE_ERR;
        }
    } else {
        tapeSize = DEFAULT_TAPE_SIZE;
    }
#else
    uintmax_t tapeSize;
    
    if (argc == 3) {
        errno = 0;   /* strto* functions don't set it to 0 for us. */
        const char *const str = argv[2];
        char* end;

        tapeSize = strtoumax(str, &end, 10);
        
        if (end == str || *end != '\0' ||
            (errno == ERANGE && (tapeSize == 0 || tapeSize == UINTMAX_MAX))) {
            fprintf(stderr, "Tape size out of range. Tape size must be greater than 0 and less than or equal to %zu", SIZE_MAX);
            return USAGE_ERR;
        }
    } else {
        tapeSize = DEFAULT_TAPE_SIZE;
    }
#endif

In the switch statement, there are no checks for overflow. What happens when the size of the file exceeds tapeSize? For instance, if I try to input a file containing 31000 bytes of < followed by 500 bytes of +, all I get is a segmentation fault:

$ printf "%0.s<" {1..31000} > in.bf 
$ printf "%0.s+" {1..500} >> in.bf 
$ ./bf in.bf 

Segmentation fault (core dumped)

Declare variables where they are needed:

Starting from C99, variables do not have to be declared at the beginning of a function (or block); instead, you can declare them at the point where they are needed within the function's body.

fopen() is not required to set errno:

The error-handling is inconsistent. At times, we are using perror(), and at times we're calling fputs() with an error message. Consider:

#if 0
    fp = fopen (filePath , "rb");

    if(!fp) {
        perror(filePath);
        exit(IO_ERR);
    }
#else
    errno = 0;
    fp = fopen(filePath, "rb");
    
    /* The cast void is required as ISO C forbids conditional expr with only one
     * void side.
     */
    if (!fp) {
        errno ? perror(filePath) : (void) fputs("Error - failed to open file.", stderr);
        exit(IO_ERR); 
    }
#endif

Use a compound literal for simplification:

As we're using C99, this:

 SourceFile sourceFile = {
        buffer,
        lSize
    };
    
return sourceFile;

can be simplified to just:

return (SourceFile) {buffer, lSize};

Noise:

const serves no purpose with const SourceFile readSourceFile in the declaration const SourceFile readSourceFile(const char* filePath). It is ignored.

Send error messages to stderr:

if (argc < 2) {
        // puts("Error! Expected path to source file as command line argument.");
        fputs("Error! Expected path to source file as command line argument.\n", stderr);
        return USAGE_ERR;
}

Note that fputs() doesn't automatically append a newline like puts().

Minor:

    /* 
    if we have 3 arguments, 3rd argument will be the tape size in bytes
    we will try and parse it as a size_t
    */

Does it matter if there are more than 3 arguments? Currently, the interpreter is ignoring them.

Use more const:

In main():

// char* str = argv[2];
const char *const str = argv[2];

Ignoring the return of malloc() and family risks invoking undefined behavior:

// Initial tape of memory and zero it all
    uint8_t* tape = (uint8_t*)malloc(tapeSize * sizeof(uint8_t));
    memset(tape, 0, tapeSize * sizeof(uint8_t));

malloc() and family returns NULL to indicate failure. The returned void * need not be casted as there is an implicit conversion to and from a void * to any other pointer type in C.

    /* Allocate to the referenced object, not the type. 
     * It is easier to maintain.
     */
    uint8_t *const tape = malloc(tapesize * sizeof *tape);
    
    if (!tape) {
        complain();
        ..
    }

As suggested in the comments, malloc() + memset() can be replaced with calloc().

The return values of fseek() and ftell() are also ignored.

getchar() returns an int, not a uint8_t:

     // ??
     case ',': {
         tape[tapePosition] = getchar();
         break;
     }

Formatting:

The code seems manually formatted. Consider using an automatic formatter (perhaps GNU indent) and limiting the line length to 80.

Aside:

We are missing a \n in all the calls to fprintf() and fputs().

I'd expect the exit status to be EX_NOINPUT if the input file did not exist or was not readable.

Question:

How will the user ever learn about the optional third argument? The usage message doesn't say anything about it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.