Fri 14 Nov 2014 08:45:40 PM UTC, comment #8:
> The code I found HAD a cast to (void):
> (void) fscanf(file, "%" MAXSTRING_S "s%*[^\n]\n", string);
> But nevertheless gcc issued the warning.
> That's why I submitted the patch.
Merely looking at formal aspects of the code and shutting up the compiler is worse than useless, you need to understand what it does and identify and fix actual bugs.
For example, the specific fscanf() call you are citing is buggy. If the gremlinfile is completely empty - as in:
.GS
file /dev/null
.GE
then fscanf returns EOF, which is ignored, and the uninitialized content of string[] is inspected. If that, by unlucky chance, happens to be "gremlinfile", the wrong error message gets printed: "error in file format" instead of "is not a gremlin file".
Arguably, that's not a very severe error. The next one, caused by one of the other unchecked fscanf()s you are sweeping under the carpet, is worse. If the first coordinate line starts with a non-numeric character, the problem is ignored, and x and y are used uninitialized. Try the following gremlinfile:
gremlinfile
1 0.0 0.0
6
x100.0 100.0
100.0 200.0
200.0 200.0
200.0 100.0
-1.0 -1.0
5 1
0
-1
The upper left corner of the square ends up in a random location.
Besides, by putting invalid content into the gremlinfile, it's easy to provoke segfaults; i didn't look deeper into that, but that would no doubt be worthwhile.
THIS CODE IS BUGGY AS HELL. By all means, fix it if you want to. But don't blindly put lipstick on the pig, merely hiding the bugs.
Don't blindly believe in static analysis tools (including compilers). They are not gods. Sometimes, their rantings make little sense. They are mostly useful to detect subtle, well-hidden issues in code that already is of good quality - but even then, you have to ignore a false positive now and then. When dealing with crap that was never audited at all, you find more errors by merely reading the code, without letting any static analysis tool distract you.
|