remzi-arpacidusseau / ostep-projects

Projects for an undergraduate OS course
4.34k stars 1.22k forks source link

concurrency-webserver compiles with many warnings, has dangerous code #23

Open mikeblas opened 3 years ago

mikeblas commented 3 years ago

The concurrency-webserver directory builds, but produces about 15 warnings. The makefile specifies -Wall, but there's some quite dangerous code in the examples, and the compiler rightfully complains about it.

Here's the GCC version I'm using, on Ubuntu 20.04 LTS:

$ gcc --version
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

The errors are mostly about unchecked use of sprintf():

wclient.c:37:21: warning: ‘host: ’ directive writing 6 bytes into a region of size between 1 and 8192 [-Wformat-overflow=]
   37 |     sprintf(buf, "%shost: %s\n\r\n", buf, hostname);
      |                     ^~~~~~

and dubious behaviour expected of sprintf():

spin.c: In function ‘main’:
spin.c:41:13: warning: passing argument 1 to restrict-qualified parameter aliases with argument 3 [-Wrestrict]
   41 |     sprintf(content, "%s<p>My only purpose is to waste time on the server!</p>\r\n", content);
      |             ^~~~~~~                                                                  ~~~~~~~

Of course, those are just two of the 15 or so messages I get.

Students and readers will be baffled (or at least distracted) by these errors, so they should be fixed. It doesn't seem particularly good to present example code that allows buffer overruns.

mikeblas commented 3 years ago

I've posted Pull request #24 , which addresses uses of sprintf() where the destination buffer is the same as one of the source strings. That fixes four warnings, if I'm counting correctly.

The remaining warnings are from un-checked string lengths. These are pretty egregious -- buffer overflows in a server. Addressing them would mean writing lots of code to handle dynamically allocated buffers, or truncating messages if they don't fit in the remaining statically sized buffer space. Either way, it seems like they'd get in the way of code clarity.

This project doesn't seem particularly active, so maybe I won't get an answer: but what's the opinion on fixing them?