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

[G. Branden Robinson]

Update of bug #60964 (project groff):

                  Status:                    None => In Progress            
             Assigned to:                    None => gbranden               
                 Summary: [PATCH] hpftodit.cpp: warnings about a too short
array => [hpftodit] potential overrun of static buffer

    _______________________________________________________

Follow-up 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)
[...]



    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/