Hacker News new | ask | show | jobs
by pobrn 1063 days ago
It's a nice project, but - I guess as is tradition with the majority of C projects - it has resource leaks and buffer overflows. There is at least one resource leak, namely, `sfd` is not closed when certain `WARN()` invocations jump back to the `start` label; for example, when `gethostbyname()` fails (i.e. try a non-existent domain and observe that the sockets remain open with `lsof`). (It also seems to be leaked in the happy path, so presumably `SSL_set_fd()` does not take ownership.) And if the user simply presses enter, i.e. the input is an empty line, there is a buffer underflow in line 55 as `j` will be -1 initially.

Also

    addr.sin_addr.s_addr = *((unsigned long*)he->h_addr_list[i]);
is a potential buffer overflow where `long` is 64 bits since only the first four bytes of `h_addr_list[i]` can be accessed, and also potentially misaligned for `unsigned long`; and it will also not work correctly on big endian platforms where `long` is 64 bits. Using `memcpy()` would have avoided all these problems. I am really confused as to how you arrived at the conclusion that "yes, this is the way to copy 4 bytes from A to B".

This sounds rude, I know, and I apologize; I don't want to single you/this project out personally. I am just frustrated that even today there are people who work on C/C++ projects seemingly without having made GCC's -fanalyzer/asan/ubsan/valgrind/etc. an important part of their development workflow.

2 comments

This is such a great feedback. Thank you a lot. OFC I will try to correct my mistakes.

Not to make any excuses, just for the context. I'm a beginner in C programming and I wrote only few small programs. My tooling is basically non existent. I will try to improve tho.

Try "valgrind" to check potential memory leaks and also compile your C projects with -Wall and -Wextra, -pedantic it's fine too.
There's a page on the net somewhere, which was on HN front-page a while ago too, that listed all the useful warning flags for gcc, with an explanation.

I'd look for it, as a C programmer. On phone now, so sadly don't have it handy.

If you or someone else finds the link to this, I would love this!
Not OP, but the one that sticks in my mind¹ with comments². Although, I also could've sworn I commented on it :/

¹ https://nullprogram.com/blog/2023/04/29/

² https://news.ycombinator.com/item?id=35758898

I looked into those points and:

1. `sfd` is now closed. This cost me an extra line of code tho so I had to improvise.

2. `j` was not initially -1 as even empty input contains one "\n" character. But it could go pass value of 0 underflowing the buffer when buffer contained only white space characters (like in case of single "\n"). This was corrected.

3. `h_addr_list[i]` about this one. I'm quite sure I took it from example in one of man pages. Anyway, I replaced it with `memcpy`.

I also tried to use valgrind. This is OFC my first step but I will continue my studies on detecting memory leaks in future projects.