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:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.24.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Fri 26 Jul 2024 06:46:21 PM UTC, comment #4: 


commit a4fc074b36b2a6054608eb2f7e83d7b5803b8b58
Author: Alejandro Colomar <alx@kernel.org>
Date:   Sat Mar 16 13:35:11 2024 +0100

    [indxbib]: Collapse related tests.

    * src/utils/indxbib/indxbib.cpp (check_integer_arg): Collapse related
      tests.

    Fixes: d7b36a45fc3f ("[indxbib]: Mitigate Savannah #65452.")
    Link: <https://savannah.gnu.org/bugs/?65452>
    Cc: "G. Branden Robinson" <branden@debian.org>
    Cc: Dave Kemper <saint.snit@gmail.com>
    Cc: "James K. Lowden" <jklowden@schemamania.org>
    Cc: Colin Watson <cjwatson@debian.org>
    Cc: Werner LEMBERG <wl@gnu.org>
    Cc: James Clark <jjc@jclark.com>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

commit dcf9bfbef5db9ab0286ac0cda2105616397f91d1
Author: Alejandro Colomar <alx@kernel.org>
Date:   Sat Mar 16 13:35:06 2024 +0100

    [indxbib]: Remove dead code.

    * src/utils/indxbib/indxbib.cpp (check_integer_arg): Remove dead code.
      The tests (LONG_MAX > INT_MAX && n > INT_MAX) and (n > INT_MAX) are
      equivalent.

    Fixes: d7b36a45fc3f ("[indxbib]: Mitigate Savannah #65452.")
    Link: <https://savannah.gnu.org/bugs/?65452>
    Link: <https://lists.gnu.org/archive/html/groff/2024-03/msg00065.html>
    Cc: "G. Branden Robinson" <branden@debian.org>
    Cc: Dave Kemper <saint.snit@gmail.com>
    Cc: "James K. Lowden" <jklowden@schemamania.org>
    Cc: Colin Watson <cjwatson@debian.org>
    Cc: Werner LEMBERG <wl@gnu.org>
    Cc: James Clark <jjc@jclark.com>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

commit 573dcdc12ee01dc476c1c06a8b6fe5c8f9958ad3
Author: Alejandro Colomar <alx@kernel.org>
Date:   Sat Mar 16 13:35:02 2024 +0100

    [indxbib]: Clear `errno` before `strotol()` call.

    * src/utils/indxbib/indxbib.cpp (check_integer_arg): Clear `errno`
      before calling `strtol()`.  Otherwise, `errno` may hold `ERANGE` from
      before.  See strtol(3).

    Fixes: d7b36a45fc3f ("[indxbib]: Mitigate Savannah #65452.")
    Link: <https://savannah.gnu.org/bugs/?65452>
    Cc: "G. Branden Robinson" <branden@debian.org>
    Cc: Dave Kemper <saint.snit@gmail.com>
    Cc: "James K. Lowden" <jklowden@schemamania.org>
    Cc: Colin Watson <cjwatson@debian.org>
    Cc: Werner LEMBERG <wl@gnu.org>
    Cc: James Clark <jjc@jclark.com>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

commit 655ecf086142a676252a385c1c7a8be838ae9f3a
Author: Alejandro Colomar <alx@kernel.org>
Date:   Sat Mar 16 13:34:57 2024 +0100

    [indxbib]: Don't `else` after [[noreturn]].

    * src/utils/indxbib/indxbib.cpp (check_integer_arg): Don't `else` after
      [[noreturn]].

    Fixes: d7b36a45fc3f ("[indxbib]: Mitigate Savannah #65452.")
    Link: <https://savannah.gnu.org/bugs/?65452>
    Cc: "G. Branden Robinson" <branden@debian.org>
    Cc: Dave Kemper <saint.snit@gmail.com>
    Cc: "James K. Lowden" <jklowden@schemamania.org>
    Cc: Colin Watson <cjwatson@debian.org>
    Cc: Werner LEMBERG <wl@gnu.org>
    Cc: James Clark <jjc@jclark.com>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>


 G. Branden Robinson <gbranden>
Group administrator
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: 

Follow-up discussions (and bikeshedding? gnat-straining?):

https://lists.gnu.org/archive/html/groff/2024-03/msg00060.html
https://lists.gnu.org/archive/html/groff/2024-03/msg00065.html

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

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-26 gbranden Open/ClosedOpen Closed
        Planned ReleaseNone 1.24.0
    2024-07-26 gbranden StatusNone Fixed
        Assigned toNone gbranden
    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

    Copyright © 2024 Free Software Foundation, Inc.
    Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.
    The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team.
    Page source code

    Powered by Savane 3.14-f13d.
    Corresponding source code