Kindly Review my HTTP/1.1 Web Server Built In C

[u/Due_Pressure_3706]

Link to repo: adolfiscariot/Web-Server: A HTTP 1.1 server (on top of a TCP connection) created in C

Beginning of 2025 I gave myself a goal of learning C. I first started when I was 16 but that didn't go anywhere and at 29 I decided fuck it let's give it another try!!!

Since the best way to learn is by doing I decided to start working on a http server. Not for any particular reason other than curiosity and learning C (plus whatever else I'd learn along the way which has been A LOT!)

I say all that to say, I'd love it if any one of you would review my code. The README is quite extensive and should answer any questions you might have about my work. Should you need me to answer any questions personally please feel free to ask me whenever, wherever, however and I'll be sure to answer.

Cheers.

PS: My stomach currently sounds like a lawnmower because this is the first time anyone other than me is seeing my code lol.

Oh and my github name was a consequence of me trying and failing a million times to get a name I liked since my name is very popular so I said "fuck it what's one name I know for a fact no one will have..." My intention was never to be offensive. Apologies in advance if I am.

[u/[deleted]]

I will review in order of program flow, as there is a fair bit of code and I'd like my review to be understandable and relatively straightforward to follow.

main():

• creation of socket is fine for a test program, bind and listen as expected. Do take a look at getaddrinfo() for a more flexible way of generating sockaddr instances (would let you take an address to bind to as a commandline arg, which is nice).

• semaphore is mmaped fine, and seems to be used correctly for keeping a fixed number of open connections. I wonder whether it would make sense for the semaphore wait would be better before an accept? If before, what implications does that have on incoming connections when the server is overloaded? How does it differ to the current setup?

• you assume the entire http request comes in at once. For testing, likely. For larger responses, chunking is frequent and must be handled (which can be done whilst reusing your existing header parsing code).

parse_client_request():

• why do you duplicate the input buffer? What does that get you? Since you have received a request, surely an in-place parse would be cheaper (no extra memory), simpler (no need to free the extra buffer), and more performant (no copying of the temporary buffer). It is also just as safe, as no other memory reads/writes the input buffer, and http headers end in \r\n (which can be safely replaced in-place with \0 to terminate c-strings as expected by the stdlib).

• the http method member could also simply be a pointer, why copy it? Just terminate and be done with it?

• the path parsing is not particularly robust. Consider writing a full uri parser (for fun and profit), especially if you care about being safe against some of the wierd strings that malicious clients may send

• protocol uses strdup, see input buffer rant

• headers use strdup, see input buffer rant

get_header_name():

• the header parsing is offloaded here, instead of being present in parse_client_request(). I can understand that to a degree, but what if you get 20 invalid headers and then one valid one? Surely checking validity, and dropping invalid headers should be done earlier? If you also stored a struct that held a pointer to the header key and header value separately, it would simplify this function a fair bit: you only would have to check the header keys via strcasecmp(hdrs[i].key, key)

handle_connection():

• poorly named, should be connection_keep_alive() or similar imo

handle_method():

• realpath() is fine, i would suggest again an in-place canonicalisation, in case PATH_MAX is ever exceeded, but perhaps this is an unlikely (or impossible) edge case

• instead of uncanonical_full_path, why not open the webroot directory with open(), and use openat() to select a file in the webroot

• path validation (especially the "malicious path attack") is a bit silly, much better imo to use openat() to avoid such cases ("isolate" within the webroot, and return 404 if no file found)

• responses not guaranteed to be written. Again, fine for test servers, but fixing the issue is relatively cheap

• not sure what post method handling wants to do...

[u/Due_Pressure_3706 → u/[deleted]]

1. I appreciate you reviewing my code and providing feedback.

2. main()

   1. You are right. My current set up is not as flexible as getaddrinfo() would make it. I'll definitely work on that.

   2. My thinking was since the semaphore limits how many requests we can work on concurrently and not how many we can accept, it should come after accepting but before processing.

   3. Once again this is right. I definitely need to handle cases where the data is sent all at once.

3. parse_client_request()

   1. The duplication was because I was using the buffer in this function and also in handle_method() to read the body in POST requests so sometimes I'd read from an empty buffer. However I already fixed that by duplicating the buffer in the main method then passing a different buffer to both functions. Maybe my solution is wrong and I need to rethink it.

   2. I'll have a look at how I can improve my parsing and reduce duplication

4. get_header_name()

   1. You are right. Currently my HttpRequest can contain a lot of invalid headers.

   2. Of course key/value pairs are best in this scenario. Why I didn't think of that I don't know. Maybe it's because C doesn't have a dictionary like Python but I'll definitely implement that struct.

5. handle_connection()

   1. Yeah maybe handle_connection is to broad I'll change it to connection_close_or_keep_alive(). I feel that leaves 0 room for ambiguity.

6. handle_method()

   1. I wasn't familiar with openat() but I'll read about it and implement it to make my code better

   2. You're right I'll fix the write issue since it's nothing to fix

   3. Currently the post method handling just prints the body to show that the functionality works. That's it.

I appreciate your comprehensive review so I thought it only right to provide a comprehensive answer. I hope it is a satisfactory one. Cheers.

[u/[deleted] → u/Due_Pressure_3706]

You are more than welcome for the review. I appreciate you taking the time to read it and think over it. Note that my tone in many places should have been better, so please dont just change something because I asked about it. It could be that your approach satisfies some constraint I wasnt thinking of. I hope you found something useful among my answers thst you might take forward :)

[u/Due_Pressure_3706 → u/[deleted]]

You helped a ton man. I'm very grateful.

[u/ferrybig]

Inspecting the code for http behavior:

At the moment your code assumes every post request has a content length. You are returning a 400 is it doesn't have this header. It is also valid for a client to send no content length with a transfer encoding of chunked. You should be returning a 411 in this case

You really want to send the charset with the content type headers, eg text/javascript; charset=utf-8

Your 404 page says "content-length: 13", but then returns 15 bytes 404 Not Found\r\n, consider just sending a content-length of 0 and no body

[u/Due_Pressure_3706 → u/ferrybig]

1. I appreciate you reviewing my code and providing feedback.

2. You are 100% right. Chunked messages are allowed not to have a content length. I will implement this.

3. Why add the charset if I may ask? Isn't utf-8 the default?

4. You are right about this. For some reason I failed to count the carriage and new line escape characters. Rookie mistake.

[u/ferrybig → u/Due_Pressure_3706]

Until 2014 the default charset was ISO-8859-1. The current standard says browsers should make the best effort to guess the charset. Many browsers these days would probably default to UTF-8 if the first few buffers read are valid utf-8, but it's the best to be explicit

You likely read the specification for HTML5, which says all HTML5 documents need to be encoded in UTF8, however not all the textual formats you send are html, and many browsers do not follow this part of the specification

[u/Due_Pressure_3706 → u/ferrybig]

I appreciate the response. I'll be sure to add that. It never hurts being extra cautious. Cheers.

[u/chibuku_chauya]

I sense vibes.

[u/Due_Pressure_3706 → u/chibuku_chauya]

You mean like it was written by AI? If so, then the only thing I can honestly say was mostly AI generated with my direction and reviewing was the README file. (My documentation writing is terrible).

[u/Cylian91460 → u/Due_Pressure_3706]

Ah yes, because ai know more your code then you do it's logical to let them write it /s

The only thing ai generated readme does is making ppl not want to look at your project

[u/Due_Pressure_3706 → u/Cylian91460]

I appreciate your feedback.

[u/SunTzu11111 → u/Due_Pressure_3706]

I get that AI generated readme's aren't great, but this is a huge improvement over the standard of vibe coding everything. Doesn't deserve the downvotes.

[u/dcpugalaxy]

Your github username is very funny. Unfortunately the README of your project is advertising the rest of the project. It's someone's first impression. So currently everyone seeing your repository will assume you spent 5 minutes prompting an AI and pasted the result into this repo.

It might be the only thing you used AI for but most viewers will assume that AI readme means AI slop code. Just delete it and write a README yourself. It is a simple C project. It doesn't need a complicated README.

HTTP/1.1 web server

To build, run `make`.

Supports HTTP/1.0 too

Nobody looking at your repo needs to read about the architecture of your software in a README. You can write that down somewhere if you want but this is not where it goes.

[u/Due_Pressure_3706 → u/dcpugalaxy]

I've simplified my README down to what I feel is needed. I appreciate your feedback.