vkoskiv / c-ray

c-ray is a small, simple path tracer written in C
MIT License
797 stars 44 forks source link

Fix segmentation fault at exit #70

Closed madmann91 closed 4 years ago

madmann91 commented 4 years ago

This commit fixes a segmentation fault happening upon program exit. The code could be improved by making copyString return the new string instead of modifying an existing string pointer (or using strdup, as it's apparently going to be standardized in C2x). Also note that free already tests that the argument is not NULL and that thus, pretty much all calls to free that look like:

if (ptr)
  free(ptr);

can just be replaced with:

free(ptr);

In any case, this commit does not apply those stylistic changes: These are suggestions and feel free not to apply them if you don't like them.

vkoskiv commented 4 years ago

The stylistic changes you suggested make sense, and I'll apply them. I'm actually not sure why I originally wrote it like that. I'm guessing I confused freeing a NULL pointer with doing a double-free, which is why I added those rather pointless checks.

Your suggestion about reworking copyString(), again, makes a whole lot of sense, I'll change the API as you suggested. It'll be way cleaner that way. Thanks so much for taking the time to contribute! It's really great to receive good feedback like this.

madmann91 commented 4 years ago

I just run bin/c-ray scene/input.json. It can be that it runs on your machine, maybe the memory allocator on MacOS is more tolerant of this kind of problems. In general I would recommend running valgrind, as it will tell you if you try to free non-heap allocated data. On a side note, it will also tell you that in your OBJ parser there's a bunch of conditionals that depend on non-initialized data, for instance. I haven't fixed that, as it turns out it doesn't impact correctness, but it's something to watch out for.

vkoskiv commented 4 years ago

You're right about valgrind, I only occasionally run it, as it's quite annoying to get to work on macOS properly. I should use it more regularly going forward.