Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On 12/29/20 11:34 AM, Adhemerval Zanella wrote:
>                idx_t len = strlen (end);
> +              if (INT_ADD_OVERFLOW (len, n))
> +                {
> +                  __set_errno (ENAMETOOLONG);
> +                  goto error_nomem;
> +                }


The other patches in this glibc patch series look good to me. However, 
this patch has some problems. First, the overflow check does not handle 
the case where strlen (end) does not fit into len. Second, ENAMETOOLONG 
is not the right errno; it should be ENOMEM because not enough memory 
can be allocated (this is what scratch_buffer, malloc, etc. do in 
similar situations). Third (and less important), the overflow check is 
not needed on practical 64-bit platforms either now or in the forseeable 
future.

I installed the attached patch into Gnulib to fix the bug in a way I 
hope is better. The idea is that you should be able to sync this into 
glibc without needing a patch like the above.


[-- Attachment #2: 0001-canonicalize-fix-ptrdiff_t-overflow-bug.patch --]
[-- Type: text/x-patch, Size: 5658 bytes --]

From b4e94717557545d613bca58a27d4ef698d551ed2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 29 Dec 2020 17:08:11 -0800
Subject: [PATCH] canonicalize: fix ptrdiff_t overflow bug

Problem reported by Adhemerval Zanella in:
https://sourceware.org/pipermail/libc-alpha/2020-December/121182.html
* lib/canonicalize-lgpl.c, lib/canonicalize.c:
Include intprops.h.
(NARROW_ADDRESSES): New constant.
* lib/canonicalize-lgpl.c (realpath_stk):m
* lib/canonicalize.c (canonicalize_filename_mode_stk):
Work even if strlen (END) does not fit in idx_t, or if adding
N to it overflows.
* modules/canonicalize, modules/canonicalize-lgpl (Depends-on):
Add intprops.
---
 ChangeLog                 | 15 +++++++++++++++
 lib/canonicalize-lgpl.c   | 12 +++++++++++-
 lib/canonicalize.c        | 12 +++++++++++-
 modules/canonicalize      |  1 +
 modules/canonicalize-lgpl |  1 +
 5 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d03007b3e..0ef300f0b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2020-12-29  Paul Eggert  <eggert@cs.ucla.edu>
+
+	canonicalize: fix ptrdiff_t overflow bug
+	Problem reported by Adhemerval Zanella in:
+	https://sourceware.org/pipermail/libc-alpha/2020-December/121182.html
+	* lib/canonicalize-lgpl.c, lib/canonicalize.c:
+	Include intprops.h.
+	(NARROW_ADDRESSES): New constant.
+	* lib/canonicalize-lgpl.c (realpath_stk):m
+	* lib/canonicalize.c (canonicalize_filename_mode_stk):
+	Work even if strlen (END) does not fit in idx_t, or if adding
+	N to it overflows.
+	* modules/canonicalize, modules/canonicalize-lgpl (Depends-on):
+	Add intprops.
+
 2020-12-28  Bruno Haible  <bruno@clisp.org>
 
 	havelib: Fix for Solaris 11 OpenIndiana and Solaris 11 OmniOS.
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 04fe95253..e8b10f0e7 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -40,6 +40,7 @@
 #include <eloop-threshold.h>
 #include <filename.h>
 #include <idx.h>
+#include <intprops.h>
 #include <scratch_buffer.h>
 
 #ifdef _LIBC
@@ -85,6 +86,10 @@
 # define IF_LINT(Code) /* empty */
 #endif
 
+/* True if adding two valid object sizes might overflow idx_t.
+   As a practical matter, this cannot happen on 64-bit machines.  */
+enum { NARROW_ADDRESSES = IDX_MAX >> 31 >> 31 == 0 };
+
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT false
 #endif
@@ -338,7 +343,12 @@ realpath_stk (const char *name, char *resolved,
               idx_t end_idx IF_LINT (= 0);
               if (end_in_extra_buffer)
                 end_idx = end - extra_buf;
-              idx_t len = strlen (end);
+              size_t len = strlen (end);
+              if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
+                {
+                  __set_errno (ENOMEM);
+                  goto error;
+                }
               while (extra_buffer.length <= len + n)
                 {
                   if (!scratch_buffer_grow_preserve (&extra_buffer))
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index a4d3aab96..eee3dbee6 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -29,6 +29,7 @@
 
 #include <filename.h>
 #include <idx.h>
+#include <intprops.h>
 #include <scratch_buffer.h>
 
 #include "attribute.h"
@@ -43,6 +44,10 @@
 # define IF_LINT(Code) /* empty */
 #endif
 
+/* True if adding two valid object sizes might overflow idx_t.
+   As a practical matter, this cannot happen on 64-bit machines.  */
+enum { NARROW_ADDRESSES = IDX_MAX >> 31 >> 31 == 0 };
+
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT false
 #endif
@@ -389,7 +394,12 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
               idx_t end_idx IF_LINT (= 0);
               if (end_in_extra_buffer)
                 end_idx = end - extra_buf;
-              idx_t len = strlen (end);
+              size_t len = strlen (end);
+              if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
+                {
+                  errno = ENOMEM;
+                  goto error;
+                }
               while (extra_buffer.length <= len + n)
                 {
                   if (!scratch_buffer_grow_preserve (&extra_buffer))
diff --git a/modules/canonicalize b/modules/canonicalize
index 5003f2682..a6cf76f17 100644
--- a/modules/canonicalize
+++ b/modules/canonicalize
@@ -19,6 +19,7 @@ free-posix
 getcwd
 hash-triple-simple
 idx
+intprops
 memmove
 mempcpy
 nocrash
diff --git a/modules/canonicalize-lgpl b/modules/canonicalize-lgpl
index a96f9011e..b8e87a607 100644
--- a/modules/canonicalize-lgpl
+++ b/modules/canonicalize-lgpl
@@ -18,6 +18,7 @@ fcntl-h           [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONI
 filename          [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
 free-posix        [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
 idx               [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
+intprops          [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
 libc-config       [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
 malloc-posix      [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
 memmove           [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-- 
2.27.0