jstimpfle / language

The blunt programming language
4 stars 1 forks source link

Possible null pointer usage in memcpy() call. #2

Open pjmlp opened 4 years ago

pjmlp commented 4 years ago

After going through with clang tidy, there was at least a possible memory corruption error that was found.

language-master\src\elf64.c(521,9): warning GE6DAE51F: Null pointer argument in call to memory copy function [clang-analyzer-unix.cstring.NullArg]
        memcpy(t->buf + t->size, str, len + 1);
        ^
language-master\src\elf64.c:592:9: note: Calling 'init_ElfStringTable'
        init_ElfStringTable(&shstrtabStrings);
        ^
language-master\src\elf64.c:495:9: note: Null pointer value stored to 'shstrtabStrings.buf'
        t->buf = NULL;
        ^
language-master\src\elf64.c:592:9: note: Returning from 'init_ElfStringTable'
        init_ElfStringTable(&shstrtabStrings);
        ^
language-master\src\elf64.c:599:9: note: Calling 'append_to_ElfStringTable'
        append_to_ElfStringTable(&shstrtabStrings, "");
        ^
language-master\src\elf64.c:510:13: note: Assuming the condition is false
        if (t->size + len + 1 > t->allocated_size) {
            ^
language-master\src\elf64.c:510:9: note: Taking false branch
        if (t->size + len + 1 > t->allocated_size) {
        ^
language-master\src\elf64.c:521:9: note: Null pointer argument in call to memory copy function
        memcpy(t->buf + t->size, str, len + 1);
        ^

Might be a false positive, as everything else was pretty ok.

jstimpfle commented 4 years ago

Thanks for spotting. This is not really a possible memory corruption error, but it technically is Undefined Behaviour (which could manifest itself as corrupting memory or doing whatever, at runtime), since arrays given by pointer + length must point to valid memory even when length=0 due to some incredible stupidity in the C spec.

I'm not aware of a case where that actually breaks an existing compiler, and I deeply feel that the code is "right" as is, so I'm not making it wrong by adding a conditional just to fix an issue that isn't one.

I should have used my own existing copy_mem() function, though, to simply avoid these stupid kinds of issues, or at least confine these issues to a central place in case efficiency matters (it doesn't).

The code is currently collecting dust while I'm mentally preparing to possibly attempting an improved version of the project, which initially won't even include code generation or ELF handling, but probably include a simple interpreter to allow for quicker design iteration. So I'm not fixing this for the time being.