4 ms·
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
by pobrn 3y 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.
- ir3k 3y agoI 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.
- ir3k 3y agoThis 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.
- anthk 3y agoTry "valgrind" to check potential memory leaks and also compile your C projects with -Wall and -Wextra, -pedantic it's fine too.
- lelanthran 3y agoThere'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.
- onsclom 3y agoIf you or someone else finds the link to this, I would love this!
- JNRowe 3y agoNot 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://nullprogram.com/blog/2023/04/29/ ² https://news.ycombinator.com/item?id=35758898 https://news.ycombinator.com/item?id=35758898