bugGNU roff - Bugs: bug #65452, [indxbib] possibly incomplete...

 
 

bug #65452: [indxbib] possibly incomplete bounds check after strtol(3)

Submitter:  Dave <barx>
Submitted:  Mon 11 Mar 2024 05:41:01 PM UTC
   
 
Category:  Utilities Severity:  2 - Minor
Item Group:  Incorrect behaviour Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Wed 13 Mar 2024 08:46:07 PM UTC, comment #3: 

Hi Branden,

comment #2:

> commit d7b36a45fc3f49f7db82f5edd33c2a66696115e5 (HEAD -> master, origin/master, origin/HEAD)
> Author: G. Branden Robinson <g.branden.robinson@gmail.com>
> Date:   Wed Mar 13 14:50:42 2024 -0500
>
>     [indxbib]: Mitigate Savannah #65452.


I see at least a bug being introduced in that commit.  :)


@@ -343,16 +343,20 @@ static void check_integer_arg(char opt, const char *arg, int min, int *res)
 {
   char *ptr;
   long n = strtol(arg, &ptr, 10);
-  if (n == 0 && ptr == arg)
-    error("argument to -%1 not an integer", opt);
+  if (ERANGE == errno)
+    fatal("argument to -%1 must be between %2 and %3", arg, min,
+         INT_MAX);
+  else if (n == 0 && ptr == arg)
+    fatal("argument to -%1 not an integer", opt);
-verbatim

You can't check errno after strtol(3) if you haven't cleared it before.  You should add

+verbatim+
errno = 0;


right before the strtol(3) call.


Here's The Right Way to call strtol(3):
<https://www.alejandro-colomar.es/src/alx/alx/lib/liba2i.git/tree/include/a2i/strtoi.h#n29>




BTW, where's the documentation for the xstrtol() you mentioned?




On another order of things, fatal() seems to be a '[[noreturn]]' thing, even if it's not declared as such.  'else' is redundant, and you may prefer to omit it.  Among other things, it would have the benefit that this diff would have been smaller (similar to the reasons why I defend semantic newlines).




And then there's some dead code in the commit:


+    if ((LONG_MAX > INT_MAX) && (n > INT_MAX))
+      fatal("argument to -%1 must be between %2 and %3", arg, min,
+           INT_MAX);


Since you already checked for ERANGE previously, there's no reason to check for (LONG_MAX > INT_MAX).  (n > INT_MAX) is already good enough, since in the case long>int it will reject thing between INT_MAX and LONG_MAX, and in the case they're equal, the test will always be false, but so will LONG_MAX>INT_MAX.  The real test that fixed that bug was the check for errno (which is incorrect as I said earlier in this message, since you need to reset errno before the call).


Cheers,
Alex

Alejandro Colomar <alx>
Wed 13 Mar 2024 08:19:54 PM UTC, comment #2: 

Whoops, forgot to erase all the `strtoll()` evidence...


commit d7b36a45fc3f49f7db82f5edd33c2a66696115e5 (HEAD -> master, origin/master, origin/HEAD)
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Mar 13 14:50:42 2024 -0500

    [indxbib]: Mitigate Savannah #65452.

    * src/utils/indxbib/indxbib.cpp: Validate `-h` option arguments more
      carefully.

      (main): Insist on an argument value of at least 2, since a hash table
      of size 1 is pointless.

      (check_integer_arg): Try to be more robust in the fact of C/C++'s
      notorious lax integer sizing practices.  We might consider gnulib's
      "xstrtol" module.  Check `errno` for `ERANGE` after calling
      `strtoll()` and add range-oriented fatal diagnostic.  Promote other
      `-h` argument validation errors to `fatal()`.  Only perform a
      comparison against INT_MAX if LONG_MAX is larger than INT_MAX in the
      first place.  Report the supported range in range diagnostics.  Use
      C++- instead of C-style type cast of result.

    Mitigates, but arguably does not fix,
    <https://savannah.gnu.org/bugs/?65452>.  Thanks to Alex Colomar for the
    report.

    I wanted to use `strtoll()`, but...
      error: ISO C++ 1998 does not support ‘long long’ [-Wlong-long]
    ...and in any case that just kicks the can to other architectures where
    int, long, and long long are all 64 bits wide.

    gnulib, take me away...


G. Branden Robinson <gbranden>
Group administrator
Wed 13 Mar 2024 07:57:00 PM UTC, comment #1: 
G. Branden Robinson <gbranden>
Group administrator
Mon 11 Mar 2024 05:41:01 PM UTC, original submission:  

Alejandro Colomar posted this concern to the email list: http://lists.gnu.org/r/groff/2024-01/msg00054.html.  His analysis looks sound, but the code was never updated to address it.

I see some code calling strtol(3) that I suspect won't behave well in
some systems:

$ grepc -tfd check_integer_arg .
./src/utils/indxbib/indxbib.cpp:static void check_integer_arg(char opt, const
char *arg, int min, int *res)
{
  char *ptr;
  long n = strtol(arg, &ptr, 10);
  if (n == 0 && ptr == arg)
    error("argument to -%1 not an integer", opt);
  else if (n < min)
    error("argument to -%1 must not be less than %2", opt, min);
  else {
    if (n > INT_MAX)
      error("argument to -%1 greater than maximum integer", opt);
    else if (*ptr != '\0')
      error("junk after integer argument to -%1", opt);
    *res = int(n);
  }
}


I think these tests miss some corner cases:

-  If INT_MAX==LONG_MAX, then n>INT_MAX is impossible, but strtol(3)
   will return LONG_MAX and errno ERANGE for values greater than that.
   groff is silently accepting input >LONG_MAX in those systems, and
   silently saturating it to LONG_MAX (INT_MAX).

-  If min==INT_MIN==LONG_MIN, then a similar thing happens for underflow.


Dave <barx>
Group Member

 

(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 alx (Posted a comment)
  • -email is unavailable- added by gbranden (Updated the item)
  • -email is unavailable- added by barx (Submitted the item)
  • -email is unavailable- added by barx (original submitter)
  •  

    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
    2024-03-13 gbranden StatusIn Progress None
        Assigned togbranden None
    2024-03-13 gbranden StatusNone In Progress
        Assigned toNone gbranden
        SummaryPossibly incomplete bounds check after strtol(3) [indxbib] possibly incomplete bounds check after strtol(3)
    2024-03-11 barx Carbon-Copy- Added alx

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code