bugGNU roff - Bugs: bug #43569, fix compile warnings with gcc 4.6.3

 
 

bug #43569: fix compile warnings with gcc 4.6.3

Submitter:  None
Submitted:  Mon 10 Nov 2014 10:07:00 AM UTC
   
 
Category:  General Severity:  2 - Minor
Item Group:  Build/Installation Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 05 Feb 2023 06:38:16 AM UTC, comment #13: 

Several areas of the source tree were touched.

Dropping patch annotation because most or all of these were independently researched and rectified (mainly by me, I think).

We appreciate Ulrich Lauther's report.

G. Branden Robinson <gbranden>
Group administrator
Sat 16 Apr 2022 09:00:07 AM UTC, comment #12: 

Oh, I forgot to add these.  The first is also responsive to Ingo's comment #9.  The second was my response to a shock I received when testing deliberately corrupted files.


commit 9c4da4d6925431b4969fe01bac8ec4f8e51da4d4
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Apr 15 08:14:07 2022 +1000

    [grn]: Add more input validity checking.

    * src/preproc/grn/hdb.cpp (DBRead): Add more validity checking.  Verify
      that the number of conversions returned by fscanf() is as expected
      instead of throwing this information away--abort processing ("giving
      up" like pic(1) does) if it does not.  Consistently report this
      abandonment in diagnostic messages.  Similarly validate pointer
      returned by fgets().  Soften handling of invalid text condition from
      fatal errors, introduced in commit eb4f0675e, 16 August, to throw an
      error and give up.  Addresses "-Wunused-result" warnings from GCC.

commit 646539bc119e7dd838851deb77877c64d82241e7
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Apr 15 08:32:44 2022 +1000

    [grn]: Emit error when failing to open file.

    * src/preproc/grn/main.cpp (conv): Throw an error diagnostic when
      failing to open a Gremlin picture file.


G. Branden Robinson <gbranden>
Group administrator
Sat 16 Apr 2022 08:57:13 AM UTC, comment #11: 

I've fixed the last of these.


commit 363fc31eebf42a61f6eeddb423492321d0f70544
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Apr 15 15:26:50 2022 +1000

    [refer]: Quieten compiler warning.

    * src/preproc/refer/label.ypp: Drop redundant declaration of `yyparse`.
      Both byacc 20140715 and GNU Bison 3.3.2 supply the the function
      prototype themselves.  Addresses "-Wredundant-decls" warning from GCC.

    Fixes <https://savannah.gnu.org/bugs/?43569>.

    Also update editor aid comments; drop old-style Emacs file-local
    variable setting.

commit d247763cfe9e71fd453ac828d35ab8b207c962fd
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Apr 15 09:09:26 2022 +1000

    [grn]: Slightly refactor.

    * src/preproc/grn/hdb.cpp (DBGetType): Lower fatal diagnostics to errors
      when encounting invalid element type characters.  Return a value
      interpreted by our caller as an error indication instead.  Helps
      compilers determine that we're not implicitly falling through our
      cases.  Addresses "-Wimplicit-fallthrough" warnings from GCC.


G. Branden Robinson <gbranden>
Group administrator
Sun 06 Mar 2022 12:01:57 AM UTC, comment #10: 


>   for src/preproc/pic/pic.ypp
> ignoring return value of 'int system(const char*)' declared with

attribute 'warn_unused_result' [-Wunused-result]

This one's fixed now, too.


commit caa10102c9f4333a471a7a1779319e5b18dcce52
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Thu Feb 24 02:01:19 2022 +1100

    [pic]: Fix "unused result" compiler warning.

    * src/preproc/pic/pic.ypp (placeless_element) <SH>: Check return value
      of `system()` to quieten `-Wunused-result` warning; if it is less than
      zero, report it to the standard error stream.

    Also bump copyright year and add editor aid comments.


I ended up doing most of these--I guess I'll look at the rest.  I already beat on hdb.cpp some last year.

G. Branden Robinson <gbranden>
Group administrator
Thu 17 Feb 2022 01:39:36 AM UTC, comment #9: 

Already applied changes concerning the file "diff-file":

  for src/preproc/post-html.cpp fixed in

commit 8f9d3f2c88cc1fdf787c0638abb99e2cefd13e64
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Dec 31 08:03:44 2021 +1100

  for src/libs/libgroff/font.cpp fixed in

commit 8b2e692af9493d7201667485ef7ee2632cdafa5c
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Tue Aug 24 22:07:40 2021 +1000

 and

commit 66b45cef887999fbd1b20cb4c36f2f4f029caf82
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sun Sep 19 08:00:33 2021 +1000

  for src/libs/libgroff/Makefile.sub; does not exist any more.

  for src/libs/libgroff/new.cpp; is unchanged, does not show a warning
with gcc 11.2.0

####

  Warnings still shown:

  for src/preproc/grn/hdb.cpp

ignoring return value of 'int fscanf(FILE*, const char*, ...)' declared
with attribute 'warn_unused_result' [-Wunused-result]

  and

ignoring return value of 'char* fgets(char*, int, FILE*)' declared with
attribute 'warn_unused_result' [-Wunused-result]

  for src/preproc/pic/pic.ypp

ignoring return value of 'int system(const char*)' declared with
attribute 'warn_unused_result' [-Wunused-result]

Bjarni Ingi Gislason <bjarniig>
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>
Group Member
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>
Group Member
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>
Group 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>
Group Member
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>
Group Member
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 Files:
   
   
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
  • -email is unavailable- added by bjarniig (Posted a comment)
  • -email is unavailable- added by gbranden (Updated the item)
  • -email is unavailable- added by bgarrigues (Updated the item)
  • -email is unavailable- added by ulrich2612 (Updated the item)
  • -email is unavailable- added by wl (Posted a comment)
  • -email is unavailable- added by schwarze (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-02-05 gbranden CategoryCore General
        Summary[PATCH] fix compile warnings with gcc 4.6.3 fix compile warnings with gcc 4.6.3
    2022-04-16 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2022-03-06 gbranden StatusNeed Info In Progress
        Assigned toNone gbranden
        SummaryFix for compile warnings with gcc 4.6.3 [PATCH] fix compile warnings with gcc 4.6.3
    2022-01-11 gbranden Item GroupWarning/Suspicious behaviour Build/Installation
    2018-11-26 bgarrigues CategoryNone Core
        Severity3 - Normal 2 - Minor
        Item GroupNone Warning/Suspicious behaviour
    2014-11-11 ulrich2612 Attached File- Added warnings, #32415
    2014-11-11 wl StatusNone Need Info
    2014-11-10 None Attached File- Added diff-file, #32397

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code