bugGNU roff - Bugs: bug #60964, [hpftodit] potential overrun of...

 
 

bug #60964: [hpftodit] potential overrun of static buffer

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Sat 24 Jul 2021 11:51:45 AM UTC
   
 
Category:  Utilities Severity:  3 - Normal
Item Group:  Warning/Suspicious behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 27 Jul 2021 12:40:00 AM UTC, comment #2: 


commit 78220681c403a6edc4644e8919e57a2d26d13f0e
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sun Jul 25 21:08:54 2021 +1000

    [hpftodit]: Fix Savannah #60964.

    * src/utils/hpftodit/hpftodit.cpp (show_symset): Prevent sprintf() from
      overunning a static buffer.  Thanks to Bjarni Ingi Gislason for the
      report.  Resize buffer to be large enough to accommodate a 64-bit
      unsigned int type formatted as decimal.  Also add assert() before the
      sprintf() to abort if the int type is even larger than that.  Use "%u"
      conversion instead of "%d" since the quantity is unsigned.

      (hp_msl_to_ucode_name): Similar, but for a signed int.

      (unicode_to_ucode_name): Similar, but for a signed int formatted as
      (unsigned) hexadecimal.

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

    Why not use a static assert for checking the width of a primitive data
    type?  Because static assertions are a C++11 feature that did not exist
    yet in the ca. 1990 dialect of C++ that groff uses.

    Also add editor aid comments and migrate from old- to new-style Emacs
    file-local variables.


G. Branden Robinson <gbranden>
Group administrator
Sun 25 Jul 2021 09:37:24 PM UTC, comment #1: 

I'm working on this, but the patch is insufficiently paranoid for my taste, and doesn't comment why the array size was chosen.

Here's what I have in progress.


commit 78220681c403a6edc4644e8919e57a2d26d13f0e (HEAD -> master)
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sun Jul 25 21:08:54 2021 +1000

    [hpftodit]: Fix Savannah #60964.

    * src/utils/hpftodit/hpftodit.cpp (show_symset): Prevent sprintf() from
      overunning a static buffer.  Thanks to Bjarni Ingi Gislason for the
      report.  Resize buffer to be large enough to accommodate a 64-bit
      unsigned int type formatted as decimal.  Also add assert() before the
      sprintf() to abort if the int type is even larger than that.  Use "%u"
      conversion instead of "%d" since the quantity is unsigned.

      (hp_msl_to_ucode_name): Similar, but for a signed int.

      (unicode_to_ucode_name): Similar, but for a signed int formatted as
      (unsigned) hexadecimal.

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

    Why not use a static assert for checking the width of a primitive data
    type?  Because static assertions are a C++11 feature that did not exist
    yet in the ca. 1990 dialect of C++ that groff uses.

    Also add editor aid comments and migrate from old- to new-style Emacs
    file-local variables.
[...]
@@ -1256,17 +1255,19 @@ dump_symbols(int tfm_type)
 static char *
 show_symset(unsigned int symset)
 {
-   static char symset_str[8];
-
-   sprintf(symset_str, "%d%c", symset / 32, (symset & 31) + 64);
-   return symset_str;
+  // A 64-bit unsigned int produces up to 20 decimal digits.
+  assert(sizeof(unsigned int) <= 8);
+  static char symset_str[22]; // 20 digits + symset char + \0
+  sprintf(symset_str, "%u%c", symset / 32, (symset & 31) + 64);
+  return symset_str;
 }

 static char *
 hp_msl_to_ucode_name(int msl)
 {
-  char codestr[8];
-
+  // A 64-bit signed int produces up to 19 decimal digits plus a sign.
+  assert(sizeof(int) <= 8);
+  char codestr[21]; // 19 digits + possible sign + \0
   sprintf(codestr, "%d", msl);
   const char *ustr = hp_msl_to_unicode_code(codestr);
   if (ustr == NULL)
@@ -1292,8 +1293,10 @@ hp_msl_to_ucode_name(int msl)
 static char *
 unicode_to_ucode_name(int ucode)
 {
+  // A 64-bit signed int produces up to 16 hexadecimal digits.
+  assert(sizeof(int) <= 8);
   const char *ustr;
-  char codestr[8];
+  char codestr[17]; // 16 hex digits + \0

   // don't allow PUA code points as Unicode names
   if (ucode >= 0xE000 && ucode <= 0xF8FF)
[...]


G. Branden Robinson <gbranden>
Group administrator
Sat 24 Jul 2021 11:51:45 AM UTC, original submission:  

From 186ecbae7145c8ff2a45cc409242725a309460f5 Mon Sep 17 00:00:00 2001
From: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
Date: Sat, 24 Jul 2021 11:02:43 +0000
Subject: [PATCH] hpftodit.cpp: warnings about a too short array

../src/utils/hpftodit/hpftodit.cpp: In function 'char* show_symset(unsigned int)':
../src/utils/hpftodit/hpftodit.cpp:1261:25: warning: '%d' directive writing between 1 and 9 bytes into a region of size 8 [-Wformat-overflow=]
 1261 |    sprintf(symset_str, "%d%c", symset / 32, (symset & 31) + 64);
      |                         ^~
../src/utils/hpftodit/hpftodit.cpp:1261:24: note: directive argument in the range [0, 134217727]
 1261 |    sprintf(symset_str, "%d%c", symset / 32, (symset & 31) + 64);
      |                        ^~~~~~
In file included from /usr/include/stdio.h:867,
                 from ./lib/stdio.h:43,
                 from ../src/include/getopt.h:35,
                 from ../src/include/lib.h:38,
                 from ../src/utils/hpftodit/hpftodit.cpp:27:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:36:34: note: '__builtin___sprintf_chk' output between 3 and 11 bytes into a destination of size 8
   36 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   37 |       __bos (__s), __fmt, __va_arg_pack ());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  This popped up after adding the options "-ggdb -Og" to CFLAGS.

FLAGS_COMMON="\
-Wall -Wextra -Wformat=2  \
-Wstringop-overflow=4 \
-Wshadow=global -Wredundant-decls \
-Wunused \
-Wunused-parameter \
-fsanitize=signed-integer-overflow \
-fsanitize-undefined-trap-on-error \
-fstack-protector-strong -fno-common \
-fstack-clash-protection \
-ftrapv \
-funsigned-char \
-fvar-tracking-assignments  \
-ggdb -Og \
-D_FORTIFY_SOURCE=2 \
"

CFLAGS="\
$FLAGS_COMMON \
-Wmissing-prototypes \
-Wold-style-definition -Wstrict-prototypes  \
-Wold-style-declaration  \
-std=c2x"

Signed-off-by: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
---
 src/utils/hpftodit/hpftodit.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/utils/hpftodit/hpftodit.cpp b/src/utils/hpftodit/hpftodit.cpp
index 5c219113..8b330308 100644
--- a/src/utils/hpftodit/hpftodit.cpp
+++ b/src/utils/hpftodit/hpftodit.cpp
@@ -1256,7 +1256,7 @@ dump_symbols(int tfm_type)
 static char *
 show_symset(unsigned int symset)
 {
-   static char symset_str[8];
+   static char symset_str[11];

    sprintf(symset_str, "%d%c", symset / 32, (symset & 31) + 64);
    return symset_str;
--
2.30.2


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

    Date Changed by Updated Field Previous Value => Replaced by
    2021-07-27 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2021-07-25 gbranden StatusNone In Progress
        Assigned toNone gbranden
        Summary[PATCH] hpftodit.cpp: warnings about a too short array [hpftodit] potential overrun of static buffer

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code