I had Claude Code to review my Simple Web Server source code. Here was its response:

I’ll review your simple web server project. This looks like a solid learning project for systems programming! Here are the issues I found:

Critical Security Vulnerabilities

1. Path Traversal Vulnerability (parseRequest.c)

1if ((strstr(URI, "../") != 0) && (strstr(URI, "/..") != 0)) {

This logic is backwards! You’re using AND (&&) when you need OR (||). This check will never trigger because a string can’t contain both patterns simultaneously. Should be:

1if ((strstr(URI, "../") != NULL) || (strstr(URI, "/..") != NULL)) {

2. Buffer Overflow in cgiExe.c

1char res_pipe_fd_str[4];
2snprintf(res_pipe_fd_str, sizeof(res_pipe_fd_str), "%d", pipe_response[1]);

A file descriptor can be larger than 3 digits (999). Should be:

1char res_pipe_fd_str[12]; // Enough for INT_MAX

3. Command Injection via CGI Parameters

1setenv(decoded_name, decoded_val, 1);

You’re directly setting environment variables from user input without validation. Malicious input could inject shell commands if CGI scripts use system() or similar.

Fix: Validate parameter names (alphanumeric + underscore only):

1// After url_decode
2for (char *p = decoded_name; *p; p++) {
3    if (!isalnum(*p) && *p != '_') {
4        // Reject invalid parameter name
5        exit(EXIT_FAILURE);
6    }
7}

4. Race Condition in cgiExe.c

1if (pipe(pipe_stdin) == -1 || pipe(pipe_stdout) == -1 || pipe(pipe_response)) {

Missing == -1 on the third condition! Should be:

1if (pipe(pipe_stdin) == -1 || pipe(pipe_stdout) == -1 || pipe(pipe_response) == -1) {

Memory Leaks

5. Memory Leak in parseRequest.c

1char *str = strdup(req_str);

Multiple return paths don’t free str. You’re using the RETURN_RESP macro which doesn’t free it.

Fix: Either free before each return, or use a cleanup pattern:

1char *result = NULL;
2// ... do work ...
3cleanup:
4    free(str);
5    return result;

6. File Descriptor Leaks

In cgiExe.c, if fork() fails, you leak the pipe file descriptors:

1if (pid == -1) {
2    free(file_name2);
3    *resp_status = 500;
4    return RESPONSE_500;  // Pipes not closed!
5}

Fix:

1if (pid == -1) {
2    close(pipe_stdin[0]); close(pipe_stdin[1]);
3    close(pipe_stdout[0]); close(pipe_stdout[1]);
4    close(pipe_response[0]); close(pipe_response[1]);
5    free(file_name2);
6    *resp_status = 500;
7    return RESPONSE_500;
8}

Logic Errors

7. Incorrect Error Check in cgiExe.c

1if ((int)nread > 0) {
2    free(file_name2);
3    free(response);
4    *resp_status = 500;
5    return RESPONSE_500;
6}

nread is size_t (unsigned). After the loop, it will be 0 or would have errored. This check is meaningless. Should check for nread == -1 inside the loop or check errno.

8. Missing Error Handling

In handleConnection.c:

1rval = read(fd, buf, BUFSIZ);
2// perror("reading stream message");

You commented out error handling! Should be:

1if (rval < 0) {
2    perror("read");
3    close(fd);
4    exit(EXIT_FAILURE);
5}

9. Dangerous use of memset_s

1memset_s(claddr, INET6_ADDRSTRLEN, 0, INET6_ADDRSTRLEN);

FreeBSD doesn’t have memset_s in the standard library. This should be memset:

1memset(claddr, 0, INET6_ADDRSTRLEN);

Or use explicit_bzero() if you need the security guarantees.

Concurrency Issues

10. Signal Handler Race Condition

You have TWO SIGCHLD handlers:

1if (signal(SIGCHLD, reap) == SIG_ERR) { ... }
2setup_sigchld_handler();  // Sets a different handler!

The second one overwrites the first. Pick one approach.

11. Non-Async-Signal-Safe Functions

In sigchld_handler():

1void sigchld_handler() {
2    while (waitpid(-1, NULL, WNOHANG) > 0);
3}

This is fine, but your other signal handler uses wait(NULL) which doesn’t loop. Use waitpid with WNOHANG everywhere.

Resource Management

12. File Pointer Not Closed on Error

In parseRequest.c, when magic_load fails:

1if (magic_load(magic, NULL) != 0) {
2    magic_close(magic);
3    *resp_status = 500;
4    RETURN_RESP(RESPONSE_500)  // file_ptr never closed!
5}

13. Directory Not Closed on Error

In dirResponse.c:

1response = realloc(response, resp_len + footer_len + 1);
2if (response == NULL) {
3    *resp_status = 500;
4    closedir(dp);
5    return RESPONSE_500;  // Returns constant, not allocated memory!
6}

Design Issues

14. Daemon Mode Breaks Relative Paths

1daemon(-1, 0);  // Changes to / by default unless nochdir=-1

Your comment says you’re using -1 to stay in CWD, but that’s non-standard. Check FreeBSD’s daemon(3) - you might need to chdir() explicitly first.

15. CGI Script Can Hang Forever

In cgiExe.c, you read from the CGI indefinitely:

1while ((nread = read(pipe_stdout[0], buffer, BUFFER)) > 0) {

No timeout. A misbehaving CGI can hang your server’s child process forever.

Fix: Use select() or poll() with a timeout.

16. Missing Content-Length Headers

Your responses don’t include Content-Length, which can cause issues with keep-alive connections (though you use Connection: close).

Code Quality

17. Commented-Out Code

Lots of commented debug statements like:

1// printf("\tfile name: %s\n", file_name);

Remove these in production code.

18. Inconsistent Error Responses

Sometimes you return allocated strings, sometimes string literals:

1return RESPONSE_500;  // String literal

vs

1RETURN_RESP(RESPONSE_500)  // Allocated

This is dangerous and will cause double-free bugs.

19. Magic Numbers

1#define BUFFER 1024
2#define BACKLOG 5
3#define SLEEP 5

These should be configurable or at least documented why these specific values.

Summary

Critical: Fix #1 (path traversal), #3 (command injection), #4 (race condition) High: Fix memory leaks (#5, #6), signal handlers (#10), error handling (#8) Medium: Everything else

This is a good learning project! The architecture is sound (dual-stack sockets, forking model, CGI support), but it needs security hardening and error handling improvements before it’s production-ready.

Would you like me to show fixes for any specific issues?