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.
From b4e94717557545d613bca58a27d4ef698d551ed2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email protected]>
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 <[email protected]>
+
+ 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 <[email protected]>
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
