Hacker News Reader

Comment by fjfaase | Hacker News Reader

The code snippet is incorrect. I do not see a check on malloc returning null. In general that would be okay, but in an embedded environment with limited memory, that would be a serious problem.

Also, using recursion is bad, because copying a long list will lead to a stack overflow.

View on HN

It's never OK. Machines with lots of memory run out of memory too.

To present a piece of research in such trollish terms, accusing "stubborn C developers" of causing security vulnerabilities, and then present an example of code for which "we didn't even have to bother reasoning about how exactly it did its pointer manipulations" that segfaults if it runs out of memory (it dereferences the result of malloc and writes to that location 2 lines down) is beyond hilarious.

On Linux machines malloc()'s returning NULL doesn't mean you're out of memory, it means you're out of address space which is very different.

In a system with virtual memory there are a variety of limits you can hit that will prevent your process allocating more memory and cause malloc to return zero. It's normal to informally lump all these together as "running out of memory" when the distinction doesn't matter, as here.

Address space size is one of these limits, not the only one. Hitting it is rare on 64-bit operating systems, common on 32-bit ones.

Unfortunately, I’ve yet to come across a major application on linux that handles this condition gracefully.

If I understand it correctly, it is even worse, because malloc could return not NULL and still throw an exception when you try to write to the 'allocated' memory. See for example https://linuxembedded.fr/2020/01/overcommit-memory-in-linux

The tone and example code is ludicrous. If the expressed attitude is how they perceive what they are doing, their attitudes are going to be the reason their project fails.

The first line doesn’t check if is the void **r pointer is null before the dereference. The incorrectness starts there.

If the precondition states that the pointer is valid (which I expect it does), then this is not incorrect. If it does not state anything about the pointer, then just checking for NULL isn't sufficient. It'd have to explicitly state "the pointer is either valid for the duration of the function, or NULL" for this to be a worthwhile check to perform.

Yes, that is correct. The precondition for the function states that the pointer should not be null.

The specification for the listcopy is listed slightly lower down on the article as follows:

  // r :-> x ** linked_list(x,S)
  void listcopy(void **r)
  // r :-> y ** linked_list(y,S) ** linked_list(x,S)

As the precondition explicitly states that r points to a value x, it can not be NULL, so no NULL checks are needed.

I'm not sure what the sibling post is trying to imply with recursive calls though.

> I'm not sure what the sibling post is trying to imply with recursive calls though.

I think that is based on a misunderstanding of what it being passed in at the point listcopy() is called in the body of listcopy(). The pointer passed in there can't be NULL due to how it is obtained, so no further checks is necessary.

AFAICT the recursion doesn't mean the function can segfault due to a null pointer. It does however mean that it will cause a stack overflow if the list is sufficiently large, which makes it a bad implementation. It can also segfault if malloc returns zero as discussed above.

Apart from these serious runtime behaviour flaws the code is also violently unreadable, which you strangely use as a way of attacking C programmers generally, as if this code is representative of human-coded C. This implementation would fail code review for numerous reasons anywhere I've ever worked.

The article itself looks detailed and very interesting and I'll have a proper read of it if I get the time. You didn't need to make it so aggressive and clickbaity.

Reply on news.ycombinator.com