Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mikeblas opened this issue Mar 2, 2021 · 1 comment
Open

Comments

@mikeblas
Copy link

mikeblas commented Mar 2, 2021

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
Copy link
Author

mikeblas commented Mar 5, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant