bugGNU troff - Bugs: bug #43569, Fix for compile warnings with gcc...

 
 

bug #43569: Fix for compile warnings with gcc 4.6.3

Submitted by:  None
Submitted on:  Mon 10 Nov 2014 10:07:00 AM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Need Info
Privacy: PublicAssigned to: None
Open/Closed: OpenPlanned Release: None

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

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.

Ingo Schwarze <schwarze>
Wed 12 Nov 2014 11:06:35 PM UTC, comment #7:

Ingo Schwarze wrote:

> it may be appropriate to cast to (void) if a thorough audit > reveals that ignoring the return value is OK in the
> (exceptional) case at hand, usually together with a comment
> explaining why it is ok, for the next auditor.


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.

ulrich

Ulrich Lauther <ulrich2612>
Tue 11 Nov 2014 02:36:00 PM UTC, comment #6:

gcc(1) and clang(1) learned #pragma and i have found it valuable to exclude normal users from useless messages by using them.
E.g. the following is from my MUA.

/* Suppress some technical warnings via #pragma's unless developing.

  • XXX Wild guesses: clang(1) 1.7 and (OpenBSD) gcc(1) 4.2.1 don't work */

#if !defined HAVE_DEBUG && !defined HAVE_DEVEL
# if PREREQ_CLANG(3, 4)
# pragma clang diagnostic ignored "-Wunused-result"
# pragma clang diagnostic ignored "-Wformat"
# elif PREREQ_GCC(4, 7)
# pragma GCC diagnostic ignored "-Wunused-result"
# pragma GCC diagnostic ignored "-Wformat"
# endif
#endif

the precondition of that is

#ifdef _clang_
# define CC_CLANG 1
# define PREREQ_CLANG(X,Y) \
(_clang_major_ + 0 > (X) || \
(_clang_major_ + 0 == (X) && _clang_minor_ + 0 >= (Y)))
# define _EXTEN __extension_
#elif defined _GNUC_
# define CC_GCC 1
# define PREREQ_GCC(X,Y) \
(_GNUC_ + 0 > (X) || (_GNUC_ + 0 == (X) && _GNUC_MINOR_ + 0 >= (Y)))
# define _EXTEN __extension_
#endif

#ifndef CC_CLANG
# define CC_CLANG 0
# define PREREQ_CLANG(X,Y) 0
#endif
#ifndef CC_GCC
# define CC_GCC 0
# define PREREQ_GCC(X,Y) 0
#endif
#ifndef __EXTEN
# define __EXTEN
#endif

Something similar would surely make sense for groff.

Anonymous
Tue 11 Nov 2014 01:49:59 PM UTC, comment #5:

> However, the warning
> version.cpp:3:24: warning: `Version_string' initialized
> and declared `extern' [enabled by default]
> SHOULD be fixed, as it is a syntactical error.
> The line is generates in
> groff-1.22.3/src/libs/libgroff/Makefile.sub


You are essentially reverting the libgroff/Makefile.sub part of:

http://git.savannah.gnu.org/cgit/groff.git/commit/?id=b1b253a4e1fe8ae3c2071c88b5d1e19ef6eb8968

Maybe that's ok (I'm not sure).

> Question: what is the correct way to suppress a warning
> if a return value is really not needed?


That can't be answered in one word. Not every compiler warning can be suppressed in any reasoable way, some compilers are just excessively chatty. In some cases - only for functions like snprintf() where ignoring the return value is usually a severe error - it may be appropriate to cast to (void) if a thorough audit reveals that ignoring the return value is OK in the (exceptional) case at hand, usually together with a comment explaining why it is ok, for the next auditor. For functions like close() where it is usually no problem to ignore the return value, nothing should be done, not even a cast; doing anything would merely be a distraction from the clarity of the code.

Ingo Schwarze <schwarze>
Tue 11 Nov 2014 10:41:33 AM UTC, comment #4:

I mostly agree with the comments of Ingo Schwarze.

However, the warning
version.cpp:3:24: warning: ‘Version_string’ initialized and declared ‘extern’ [enabled by default]
SHOULD be fixed, as it is a syntactical error.
The line is generates in
groff-1.22.3/src/libs/libgroff/Makefile.sub

Question: what is the correct way to suppress a warning if a return value is really not needed?

I attach a list of the warnings I got.

Cheers

ulrich

(file #32415)

Ulrich Lauther <ulrich2612>
Tue 11 Nov 2014 05:30:14 AM UTC, comment #3:

I haven't yet looked at the proposed changes in detail, so I can't comment. However, it might be best if the OP could add the log file (compressed) that shows the warnings this gcc version omits.

Werner LEMBERG <wl>
Project Administrator
Mon 10 Nov 2014 11:24:51 PM UTC, comment #2:

To reinforce my previous comment, i just did some actual code auditing, and there do appear to be bugs that these bogus patches are going to hide.

For example, with respect to the second chunk (changing font.cpp), if a DESC file contains a "papersize" line where the referenced file cannot be read, the fgets() call touched here will fail, return NULL, and the content of the "line" buffer will remain undefined. Consequently, the following strchr() call may read off the end of the buffer, possibly resulting in a segfault or data corruption. Besides, if the file is readable but of length zero, fgets() will set line[0] to '\0' and the subsequent "if ((--linep) == '\n') linep = '\0';" is both an out of bounds read and a write access, possibly causing a different segfault or data corruption. Not 100% sure as this is just from code inspection, but the code really doesn't look correct at all.

So i strongly urge you to not commit these dangerous patches and instead fix the actual bugs. Right now, i'm not going to do the required work as i have other work to do, sorry.

NEVER change code to appease the compiler, only use compiler warnings as hints where to look for bugs, THEN FIX THE BUGS.

Ingo Schwarze <schwarze>
Mon 10 Nov 2014 10:49:16 PM UTC, comment #1:

I quite strongly disagree with these proposed changes.

Adding useless variables (like FILE* foo in post-html.cpp) harms readability and hinders code audits because people reading the code will have to figure out that these variables are unused before being able to understand the code. Besides, some compilers warn about variables assigned to but never read, so this just shifts the nagging elsewhere.

Besides, i suspect this is a case of "the compiler warns, so add some code to make it shut up" without bothering to actually understand the code at hand. That kind of a code audit is worse than no code audit at all because it can hide real bugs that might otherwise be found more easily by future auditors.

So i suggest to close this bug report as invalid.

Ingo Schwarze <schwarze>
Mon 10 Nov 2014 10:07:00 AM UTC, original submission:

gcc version 4.6.3 issues some warnings when compiling groff-1.22.3, mostly for unused return values and once for initializing a variable declared "extern".

The patch attached tries to fix this.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #32415:  warnings added by ulrich2612 (2KiB - application/octet-stream)
file #32397:  diff-file added by None (5KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by ulrich2612 (Updated the item)
  • -unavailable- added by wl (Posted a comment)
  • -unavailable- added by schwarze (Posted a comment)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 11 Nov 2014 10:41:33 AM UTCulrich2612Attached File-=>Added warnings, #32415
    Tue 11 Nov 2014 05:30:14 AM UTCwlStatusNone=>Need Info
    Mon 10 Nov 2014 10:07:00 AM UTCNoneAttached File-=>Added diff-file, #32397

    Back to the top


    Powered by Savane 3.1-cleanup1