bugGNU roff - Bugs: bug #61643, [not our problem] [grops] core...

 
 

bug #61643: [not our problem] [grops] core dump when running and compiled with -Wsanitize=undefined

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Wed 08 Dec 2021 06:29:17 PM UTC
   
 
Category:  Driver grops Severity:  2 - Minor
Item Group:  Lint Status:  Invalid
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 30 Sep 2023 07:31:48 PM UTC, comment #7: 

Thanks, Bjarni.  Updating summary.

G. Branden Robinson <gbranden>
Group administrator
Sat 30 Sep 2023 07:27:18 PM UTC, comment #6: 

  This does not occur with gcc version (Debian 13.2.0-4) 13.2.0

Bjarni Ingi Gislason <bjarniig>
Sat 17 Sep 2022 01:20:21 AM UTC, comment #5: 

I think this is a bug in ASAN, but I did change these static structs to include the "const" modifier.  Maybe that will clue ASAN in and maybe it won't.


commit 1b676f414a33964a7e3708a692a70a13d70f9a91
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Tue Sep 13 04:34:52 2022 -0500

    [grops]: Fix code style nit.

    * src/devices/grops/ps.cpp (ps_printer::special)
      (resource_manager::process_file): Fix code style nit.  Mark static
      structs initialized within functions and never modified thereafter as
      `const`.  This attempts to work around an apparent false positive from
      AddressSanitizer.  If it doesn't pacify ASAN, please explain to me how
      constant structure members initialized to string literals and function
      pointers within the translation unit can ever be null.  If that is
      infeasible, please report a bug to your compiler vendor.


G. Branden Robinson <gbranden>
Group administrator
Tue 13 Sep 2022 09:09:44 AM UTC, comment #4: 

comment #1:

>   This bug should be fixed before the next release.


I don't agree with this implication of urgency.  This looks to me like a false positive from AddressSanitizer.

Consider the data structures that are being walked here.  They are both statically initialized within functions and never modified.

https://git.savannah.gnu.org/cgit/groff.git/tree/src/devices/grops/ps.cpp#n1528

https://git.savannah.gnu.org/cgit/groff.git/tree/src/devices/grops/psrm.cpp#n971

As an experiment, I added `assert()`s to both functions.


@@ -1557,7 +1557,9 @@ void ps_printer::special(char *arg, const environment *env, char type)
     error("empty X command ignored");
     return;
   }
-  for (unsigned int i = 0; i < sizeof(proc_table)/sizeof(proc_table[0]); i++)
+  for (unsigned int i = 0; i < sizeof(proc_table)/sizeof(proc_table[0]); i++) {
+    assert(proc_table[i].name != 0 /* nullptr */);
+    assert(proc_table[i].proc != 0 /* nullptr */);
     if (strncmp(command, proc_table[i].name, p - command) == 0) {
       flush_sbuf();
       if (sbuf_color != *env->col)
@@ -1565,6 +1567,7 @@ void ps_printer::special(char *arg, const environment *env, char type)
       (this->*(proc_table[i].proc))(p, env);
       return;
     }
+  }
   error("X command '%1' not recognised", command);
 }

@@ -1023,12 +1023,15 @@ void resource_manager::process_file(int rank, FILE *fp,
        if (buf[1] == '%') {
          const char *ptr;
          int i;
-         for (i = 0; i < NCOMMENTS; i++)
+         for (i = 0; i < NCOMMENTS; i++) {
+           assert(comment_table[i].name != 0 /* nullptr */);
+           assert(comment_table[i].proc != 0 /* nullptr */);
            if ((ptr = matches_comment(buf, comment_table[i].name))) {
              copy_this_line
                = (this->*(comment_table[i].proc))(ptr, rank, fp, outfp);
              break;
            }
+         }
          if (i >= NCOMMENTS && in_header) {
            if ((ptr = matches_comment(buf, "EndComments")))
              in_header = 0;


Neither rebuilding the groff tree nor running your reproducer script provoked an assertion failure.

However, recalling above that these structs are "never modified", I think a better fix is simply to declare them `const`.  So I'll do that.

That reduces the severity of this ticket to 'minor' and moves it to item group 'Lint'.

If some C/C++ genius wants to explain to me how AddressSanitizer isn't actually false-flagging these structure member accesses, I'm all ears.

G. Branden Robinson <gbranden>
Group administrator
Tue 06 Sep 2022 12:31:52 AM UTC, comment #3: 

  In the subject and the original submission "-Wsanitize=undefined", "-W" must be "-f".

Bjarni Ingi Gislason <bjarniig>
Tue 06 Sep 2022 12:12:49 AM UTC, comment #2: 

  Forgot a reproducer


#!/bin/sh

top_srcdir=$HOME/git/groff

hdtbl_srcdir=$top_srcdir/contrib/hdtbl/


doc_buildir=$top_srcdir/build/contrib/hdtbl

doc_srcdir=$top_srcdir/contrib/hdtbl

inputfile=$top_srcdir/build/contrib/hdtbl/examples/fonts_n.roff

test-groff -t -p -e -U -I $doc_buildir -I doc_srcdir \
  -dsopath=$hdtbl_srcdir -dfontpath=$top_srcdir/font  -mhdtbl $inputfile


Bjarni Ingi Gislason <bjarniig>
Tue 06 Sep 2022 12:05:32 AM UTC, comment #1: 

  This bug should be fixed before the next release.

Bjarni Ingi Gislason <bjarniig>
Wed 08 Dec 2021 06:29:17 PM UTC, original submission:  

Subject: grops: core dump when running and compiled with -Wsanitize=undefined

traps: grops[17629] trap invalid opcode ip:555e4af69280 sp:7fff0bc51790
error:0 in grops[555e4af5d000+28000]

traps: grops[18200] trap invalid opcode ip:55a43e350280 sp:7ffcaeb8dcf0
error:0 in grops[55a43e344000+28000]

#0  0x000055a43e350280 in resource_manager::process_file
(this=0x55a43f433758, rank=0, fp=<optimized out>,
    filename=<optimized out>, outfp=<optimized out>) at
../src/devices/grops/psrm.cpp:1026
1026                    = (this->*(comment_table[i].proc))(ptr, rank,
fp, outfp);

(gdb) print i
$1 = 1

  Warnings from compilations:

../src/devices/grops/ps.cpp: In member function 'special':
../src/devices/grops/ps.cpp:1567:28: warning: 'i.130' may be used
uninitialized [-Wmaybe-uninitialized]
 1567 |       (this->*(proc_table[i].proc))(p, env);
      |                            ^
../src/devices/grops/ps.cpp:1567:28: note: 'i.130' was declared here
 1567 |       (this->*(proc_table[i].proc))(p, env);
      |                            ^
../src/devices/grops/psrm.cpp: In member function 'process_file':
../src/devices/grops/psrm.cpp:1026:43: warning: 'i.104' may be used
uninitialized in this function [-Wmaybe-uninitialized]
 1026 |                 = (this->*(comment_table[i].proc))(ptr, rank,
fp, outfp);
      |                                           ^
  CXX      src/devices/grotty/tty.o
 

Bjarni Ingi Gislason <bjarniig>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by bjarniig (Submitted the item)
  •  

    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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-09-30 gbranden Summarygrops: core dump when running and compiled with -Wsanitize=undefined [not our problem] [grops] core dump when running and compiled with -Wsanitize=undefined
    2022-09-17 gbranden StatusIn Progress Invalid
        Open/ClosedOpen Closed
        Planned Release1.23.0 None
    2022-09-13 gbranden Severity3 - Normal 2 - Minor
        Item GroupBuild/Installation Lint
        StatusNone In Progress
        Assigned toNone gbranden
        Planned ReleaseNone 1.23.0

    Back to the top

    Powered by Savane 3.13-8484.
    Corresponding source code