28 comments

[ 2.3 ms ] story [ 51.6 ms ] thread
There are many, many more such issues with that code. The person that posted it is new to C and had an AI help them to write the code. That's a recipe for disaster, it means the OP does not actually understand what they wrote. It looks nice but it is full of footguns and even though it is a useful learning exercise it also is a great example of why it is better run battle tested frame works than to inexpertly roll your own.

As a learning exercise it is useful, but it should never see production use. What is interesting is that the apparent cleanliness of the code (it reads very well) is obscuring the fact that the quality is actually quite low.

If anything I think the conclusion should be that AI+novice does not create anything that is useable without expert review and that that probably adds up to a net negative other than that the novice will (hopefully) learn something. It would be great if someone could put in the time to do a full review of the code, I have just read through it casually and already picked up a couple of problems, I'm pretty sure that if you did a thorough job of it there would be many more.

I can't completely blame the language here: anyone "coding" in a language new to them using an LLM is going to have real problems.
It's a double-sided coin. LLMs are probably the best way to learn programming languages right now. But if you vibecode in a programming language that you don't understand, it's going to be a disaster sooner or later.

This is also the reason why AI will not replace any actual jobs with merit.

> Another interesting choice in this project is to make lengths signed:

There are good reasons for this choice in C (and C++) due to broken integer promotion and casting rules.

See: "Subscripts and sizes should be signed" (Bjarne Stroustrup) https://open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1428r0...

As a nice bonus, it means that ubsan traps on overflow (unsigned overflows just wrap).

If using C23, _BitInt allows for integer types without promotion.
Good C code will try to avoid allocations as much as possible in the first place. You absolutely don’t need to copy strings around when handling a request. You can read data from the socket in a fixed-size buffer, do all the processing in-place, and then process the next chunk in-place too. You get predictable performance and the thing will work like precise clockwork. Reading the entire thing just to copy the body of the request in another location makes no sense. Most of the “nice” javaesque XXXParser, XXXBuilder, XXXManager abstractions seen in “easier” languages make little sense in C. They obfuscate what really needs to happen in memory to solve a problem efficiently.
This shared memory and pointer shuffling is of course fraught with requiring correct logic to avoid memory safety bugs. Good C code doesn't get you pwned, I'd argue.
A long time ago I was involved in building compilers. It was common that we solved this problem with obstacks, which are basically stacked heaps. I wonder one could not build more things like this, where freeing is a bit more best effort but you have some checkpoints. (I guess one would rather need tree like stacks) Just have to disallow pointers going the wrong way. Allocation remains ugly in C and I think explicit data structures are are definitely a better way of handling it.
That mythical "Good C Code", which is known only to some people who I never met.
These abstractions were already common in enterprise C code decades before Java came to be, thanks to stuff like Yourdon Structured Method.

Using fixed size buffers doesn't fix out of bounds errors, and stack corruption caused by such bugs.

Naturally we all know good C programmers never make them. /s

>Good C code will try to avoid allocations as much as possible in the first place.

there's a genius to this: if you're going to optimize prematurely, do it right out of the gate!

Agree re: no need for heap allocation - for others: I recommend reading thru whole masscan source (https://github.com/robertdavidgraham/masscan), it's a pleasure btw - iirc rather few/sparse malloc()s which are part of regular I/O processing flow (there will be malloc()s which depending on config etc. set up additional data structs but as part of setup).
Reads like an indictment of vibe coding.

LLMs are fundamentally probabilistic --- not deterministic.

This basically means that anything produced this way is highly suspect. And this framework is an example.

As an aside, it's amusing that it took 25 years for C coders to embrace the C99 named struct designator feature:

    HttpParser parser = {
        .isValid = true,
        .requestBuffer = strdup(request),
        .requestLength = strlen(request),
        .position = 0,
    };
All the kids are doing it now!
Long as you allocate me, it's alright
One thing to note, too, is that `atoi()` should be avoided as much as possible. On error (parse error, overflow, etc), it has an unspecified return value (!), although most libcs will return 0, which can be just as bad in some scenarios.

Also not mentioned, is that atoi() can return a negative number -- which is then passed to malloc(), that takes a size_t, which is unsigned... which will make it become a very large number if a negative number is passed as its argument.

It's better to use strtol(), but even that is a bit tricky to use, because it doesn't touch errno when there's no error but you need to check errno to know if things like overflow happened, so you need to set errno to 0 before calling the function. The man page explains how to use it properly.

I think it would be a very interesting exercise for that web framework author to make its HTTP request parser go through a fuzz-tester; clang comes with one that's quite good and easy to use (https://llvm.org/docs/LibFuzzer.html), especially if used alongside address sanitizer or the undefined behavior sanitizer. Errors like the one I mentioned will most likely be found by a fuzzer really quickly. :)

I definitely don't love C that does atoi on a Content-Length value that came from the network and passes that to malloc.

Even before we get to how a malicious would interact with malloc, there is this:

> The functions atof, atoi, atol, and atoll are not required to affect the value of the integer expression errno on an error. If the value of the result cannot be represented, the behavior is undefined. [ISO C N3220 draft]

That includes not only out-of-range values by garbage that cannot be converted to a number at all. atoi("foo") can behave in any manner whatsoever and return anything.

Those functions are okay to use on something that has been validated in a way that it cannot cause a problem. If you know you have a nonempty sequence of nothing but digits, possibly with a minus sign, and the number digits is small enough that the value will fit into int, you are okay.

> A malicious user can pass Content-Length of 4294967295

But why would they when it's fewer keystrokes to use -1, which will go to 4294967295 on a 32 bit malloc, while scaling to 18446744073709551615 on 64 bit?

This seems more like a coding problem than a C problem. Most people would at least validate input before allocating anything.
A HTTP request processing layer would typically have some configuration of the maximum content size that can be received in one request and reject anything larger.

That's why, e.g., you have to mess with a "php.ini" file to get your self-hosted webmail to handle large attachments (which are uploads from the browser UI to the back-end).

> But why would they when it's fewer keystrokes to use -1, which will go to 4294967295 on a 32 bit malloc, while scaling to 18446744073709551615 on 64 bit?

If that user wants to exploit your application it's better not to pass such a high value, since malloc typically detects size > SIZE_MAX/2. But then this code also doesn't check for malloc to return NULL, so this might also what leads to an exploit.

Integer operations, the one thing in computers where basically there's no non-annoying way to do them right except being over pedantic with checks
OT: using the `strcasecmp` family of functions is basically asking for trouble - unless you've previously set the locale to "C", which is basically the only locale with a defined behaviour. Otherwise you're basically bound to run onto very funny internationalisation issues you'd rather know nothing about (and fail the Turkey Test)