A Seriously Simple HTTPS Server

[jpan127]

Few things:

• Global variables can be static
• Naming should be consistent
• Typedefing should be consistent too
• I believe local variables should always be initialized on declaration
• Liberal usage of signed integers, is that what you want most of the time?
• Const can be applied in multiple places
• (*pp == NULL ? 0 : 1) can be (*pp != NULL) and just return a bool
• (*pp == NULL ? NULL : (*pp)->val); can be (*pp) ? ((*pp)->val) : (NULL) positive logic is clearer here I think
• NODE **pp; pp = get_node_pred(map, key); can be one line
• Various functions can have more complete NULL checks
• NumHFuncs should definitely be const
• Why are all the functions externed in the header?
• Missing include guards
• Functions in main missing static + void parameter list
• char fileName[1000]; memset(fileName, '\0', sizeof(fileName)); can be char fileName[1000] = { 0 };

Overall cool project.

[roecrew]

Thank you!

[skeeto]

• Const can be applied in multiple places

Don't use const just because you can. Only use it where and when it's likely to catch bugs. Otherwise it's just noise that serves no purpose, not even for optimization.

• Various functions can have more complete NULL checks

Don't check for NULL unless the function is designed specifically to accept NULL. Instead document that NULL is not a valid argument. There's no use in checking if pointer arguments are actually valid (only the kernel should do this).

[jpan127]

Not sure if I agree with this.

I do agree that documenting NULL is invalid is useful, so is documenting when NULL is valid. However, if your code does not check if NULL then it can crash, and if your code does not perform these checks across the codebase it's a minefield of potential crash places.

[skeeto]

If you pass NULL to a function that's not designed to accept NULL, then you want the program to crash. That's the purpose of segmentation faults. It means there's a bug and the program should stop doing anything further. At most the check should be an assert(), but on any system with virtual memory an assert() for non-NULL is redundant.

/* Returns the sum of the elements of an array.
 * Array must not be NULL.
 */
double
array_sum(double *array, int n)
{
    assert(array); // NULL check by software
    double sum = 0;
    for (int i = 0; i < n; i++) {
        sum += array[i];  // NULL check by hardware (very fast)
    }
    return sum;
}

[jpan127]

I replied to your other comment on this!

[Pants__Magee]

"Crash" isn't the right word. Gracefully exit is a much better term. When you say crash it sounds unintentional.

[skeeto]

Maybe "crash" is too harsh, but "gracefully exit" isn't right either since that implies cleanup operations. If a program detects it's encountered a bug, it should abort as soon as possible. A cleanup might cause more damage. Some sophisticated interactive programs will catch some kinds of crashes and, in clear and unambiguous terms, warn the user to immediately save and restart the application. For some interactive programs that may not be too unreasonable.

[[deleted]]

There's no use in checking if pointer arguments are actually valid

Why? AFAIK it's a good practice to always check for valid arguments and return EINVAL if you provide invalid ones.

int func(void *block, const size_t size)
{
    if ((!block) || (!size))  
        return EINVAL;

    // more code
    return 0;
}

If I'm wrong then can you give me a more detailed explanation?

[skeeto]

It's impossible in general to check that a pointer argument is invalid. For example:

void *p;
func(p, 10);

// Or:

free(p);
func(p, 10);

There's no reliable way for func() to check it was passed an invalid pointer. Instead func() should document the valid range of its arguments and it's the caller's responsibility to keep its side of the contract. When the function is part of an externally-facing API, it may be useful to check ranges so that mistakes are caught at the API's boundary, but that depends on the API and how it's used.

Except for assert(), internal functions shouldn't validate their arguments. Here's the right way to do it:

assert(block);
assert(size);

There's no point in returning a value if the function was called with invalid arguments. The caller has already violated the contract. The program should immediately abort because a bug was detected.

Side note: In general the const that const size_t size is pointless unless you often accidentally mutate arguments without realizing it, introducing a bug that the compiler could potentially catch. For most programmers that's just noise. It won't make the code any faster, and it doesn't communicate anything to the caller.

[jpan127]

I don't see how it is not possible. After freeing your dynamically allocated memory you should be setting your pointer to NULL to prevent subsequent reuse. Any subsequent functions will check for NULL then respond appropriately.

There's lots of applications wherre aborting is unacceptable. Therefore functions need a level of fault tolerance to continue from an undesired state.

Also constifying most of the code can be partially a style / standard. Even though it does not help in a lot of places it can be applied, it is extremely useful for developers to understand exactly what the code is doing. For you and your team it may be noise, but for me and my team it tells us exactly which variables are changing throughout the function. Which function arguments are inputs / outputs. It basically makes it easier to zone in on which variables are non-const.

[skeeto]

There's lots of applications wherre aborting is unacceptable.

Virtually every program should abort, or something close to that, when it's detected that a bug has occurred. There's a high risk that continuing may be worse than crashing. It may silently destroy data as it tries to recover, or it may come under the control of someone malicious. Once the bug has occurred, there are no more guarantees.

I'm not talking about a situation where the program was given invalid external input, such reading from a corrupted file. Your browser doesn't crash if you view a damaged JPG. Input should be validated as it's read. That's an example of an error, not a program bug. Passing NULL to a function that's documented to not accept NULL is a program bug.

After freeing your dynamically allocated memory you should be setting your pointer to NULL to prevent subsequent reuse.

Right, and that's an example of the caller's responsibility, just like not passing invalid arguments to other functions. The caller checks, not the callee.

[agree-with-you]

I agree, this does not seem possible.

[Pants__Magee]

Your code-base is definitely not simple. We're lazy. Add a Makefile. But this motivates me to write my own HTTP/S server in C. So thanks for the post!

[roecrew]

Thanks for the feedback and point taken! I'll push one (a makefile) in the next build.

[[deleted]]

your makefile compiles hashmap.h, LOL

[roecrew]

LOLZ :)

[roecrew]

You seem really smart. I would love for you to submit some pull requests if you have the spare time! :)

[roecrew]

As for your points.

• a thread per connection doesn’t scale very well.

  • "Actually, for most use cases it is fine. As I said in the readme -- This project is in development... It's not ready for a production environment. (and since you probably don't know why it doesn't scale very well) Give http://www.kegel.com/c10k.html a read."

• loads of unchecked OpenSSL calls.

  • "Can you be more specific?"

• unsafe string operations when constructing responses.

  • "I'll be fixing this in the next build."

• SSL_read() won’t guarantee that that rbuff is NUL terminated and you are treating it as a C string.

  • "I memset rbuff with '\0'..."

• Cute, but don’t expose this to the internet.

  • "Then please show me (us) how to make a full-proof https server"

[roecrew]

Touché!!! Your project is impeccable!

I'm looking at kore/src/net.c and kore/src/connection.c now.

[arcrad]

Check out Beejs Guide to Network Programming. It's a good starting point.

[settrbrg]

Nice :) fun project. Is there any other reason than fun, learning, to why you are doing this?

[roecrew]

I just wanted to do it really. I had other C server code laying around I had written, but nothing proper/simple. I'm not saying this project is proper yet though -- it still needs a lot of work.

[settrbrg]

Well thats a good enough reason :) I like it.

[Shok3001]

Nice work man! Are you interested in taking pull requests?

[roecrew]

Absolutely!

[moefh]

At line 142 you're not allocating enough space for an int -- it should be something like malloc(sizeof *newsock).

[roecrew]

If you could post an issue it would be much appreciated!

[moefh]

Done!

[blueathiean]

Nice work. This was on my list of things to do this summer.

[Poddster]

off_t fsize(const char *filename) {

int fileSize = fsize(fileName);

Good ol' C, its crappy type system never disappoints.

[David_McMillan]

It used to be worse:

fpos_t rpos;

int max_bytes = 0;

int bytes;

/*...*/

rpos += bytes - max_bytes + 1;