Add XtReallocArray() for overflow checking of multiplied args

Uses reallocarray() if available, otherwise checks for overflow itself,
if overflow is possible (i.e. in ILP32 & ILP64 environments, but not LP64
with 32-bit ints).

Includes unit tests and XtMallocArray() helper macro.

Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>

7 files changed, 264 insertions, 15 deletions

diff --git a/COPYING b/COPYING
index 30ef492..c341e9d 100644
--- a/COPYING
+++ b/COPYING
@@ -39,7 +39,7 @@ DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
 TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 PERFORMANCE OF THIS SOFTWARE.
 
-Copyright (c) 1993, 2011, Oracle and/or its affiliates.
+Copyright (c) 1993, 2023, Oracle and/or its affiliates.
 
 Permission is hereby granted, free of charge, to any person obtaining a
 copy of this software and associated documentation files (the "Software"),
diff --git a/configure.ac b/configure.ac
index e0dbaa4..1da5532 100644
--- a/configure.ac
+++ b/configure.ac
@@ -22,13 +22,18 @@
 #
 
 # Initialize Autoconf
-AC_PREREQ([2.60])
+AC_PREREQ([2.70])
 AC_INIT([libXt], [1.2.1],
 	[https://gitlab.freedesktop.org/xorg/lib/libXt/issues], [libXt])
 AC_CONFIG_SRCDIR([Makefile.am])
 AC_CONFIG_HEADERS([config.h])
 AC_CONFIG_MACRO_DIR([m4])
 
+# Set common system defines for POSIX extensions, such as _GNU_SOURCE
+# Must be called before any macros that run the compiler (like AC_PROG_LIBTOOL)
+# to avoid autoconf errors.
+AC_USE_SYSTEM_EXTENSIONS
+
 # Initialize Automake
 AM_INIT_AUTOMAKE([foreign dist-xz])
 
@@ -53,6 +58,9 @@ XORG_WITH_PERL
 # Checks for header files.
 AC_CHECK_HEADER([alloca.h], AC_DEFINE(INCLUDE_ALLOCA_H, 1, [Define to 1 if Xalloca.h should include <alloca.h>]))
 
+# Checks for library functions.
+AC_CHECK_FUNCS([reallocarray])
+
 # Obtain compiler/linker options for dependencies
 PKG_CHECK_MODULES(XT, sm ice x11 xproto kbproto)
 
diff --git a/include/X11/Intrinsic.h b/include/X11/Intrinsic.h
index 668b7d5..71199e7 100644
--- a/include/X11/Intrinsic.h
+++ b/include/X11/Intrinsic.h
@@ -1864,6 +1864,12 @@ extern char *XtRealloc(
     Cardinal 		/* num */
 );
 
+extern void *XtReallocArray(
+    void * 		/* ptr */,
+    Cardinal 		/* num */,
+    Cardinal 		/* size */
+);
+
 extern void XtFree(
     char*		/* ptr */
 );
@@ -1892,6 +1898,14 @@ extern char *_XtRealloc( /* implementation-private */
     int		/* line */
 );
 
+extern char *_XtReallocArray( /* implementation-private */
+    char *	/* ptr */,
+    Cardinal	/* num */,
+    Cardinal    /* size */,
+    const char */* file */,
+    int		/* line */
+);
+
 extern char *_XtCalloc( /* implementation-private */
     Cardinal	/* num */,
     Cardinal 	/* size */,
@@ -1911,9 +1925,15 @@ extern void _XtPrintMemory( /* implementation-private */
 
 #define XtMalloc(size) _XtMalloc(size, __FILE__, __LINE__)
 #define XtRealloc(ptr,size) _XtRealloc(ptr, size, __FILE__, __LINE__)
+#define XtMallocArray(num,size) _XtReallocArray(NULL, num, size, __FILE__, __LINE__)
+#define XtReallocArray(ptr,num,size) _XtReallocArray(ptr, num, size, __FILE__, __LINE__)
 #define XtCalloc(num,size) _XtCalloc(num, size, __FILE__, __LINE__)
 #define XtFree(ptr) _XtFree(ptr)
 
+#else
+
+#define XtMallocArray(num,size) XtReallocArray(NULL, num, size)
+
 #endif /* ifdef XTTRACEMEMORY */
 
 #define XtNew(type) ((type *) XtMalloc((unsigned) sizeof(type)))
diff --git a/man/XtMalloc.man b/man/XtMalloc.man
index 2fa1d57..bdf580b 100644
--- a/man/XtMalloc.man
+++ b/man/XtMalloc.man
@@ -36,7 +36,7 @@
 .na
 .TH XtMalloc __libmansuffix__ __xorgversion__ "XT FUNCTIONS"
 .SH NAME
-XtMalloc, XtCalloc, XtRealloc, XtFree, XtNew, XtNewString \- memory management functions
+XtMalloc, XtCalloc, XtRealloc, XtReallocArray, XtFree, XtNew, XtNewString \- memory management functions
 .SH SYNTAX
 #include <X11/Intrinsic.h>
 .HP
@@ -44,7 +44,9 @@ char *XtMalloc(Cardinal \fIsize\fP);
 .HP
 char *XtCalloc(Cardinal \fInum\fP, Cardinal \fIsize\fP);
 .HP
-char *XtRealloc(char *\fIptr\fP, Cardinal \fInum\fP);
+char *XtRealloc(char *\fIptr\fP, Cardinal \fIsize\fP);
+.HP
+void *XtReallocArray(void *\fIptr\fP, Cardinal \fInum\fP, Cardinal \fIsize\fP);
 .HP
 void XtFree(char *\fIptr\fP);
 .HP
@@ -55,7 +57,7 @@ String XtNewString(String \fIstring\fP);
 Cardinal XtAsprintf(char **\fInew_string\fP, const char *\fIformat\fP, ...);
 .SH ARGUMENTS
 .IP \fInum\fP 1i
-Specifies the number of bytes or array elements.
+Specifies the number of array elements.
 .IP \fIptr\fP 1i
 Specifies a pointer to the old storage or to the block of storage that is to be freed.
 .IP \fIsize\fP 1i
@@ -90,17 +92,18 @@ calls
 .LP
 The
 .B XtRealloc
-function changes the size of a block of storage (possibly moving it).
-Then, it copies the old contents (or as much as will fit) into the new block
-and frees the old block.
+and
+.B XtReallocArray
+functions change the size of a block of storage (possibly moving it).
+Then, they copy the old contents (or as much as will fit) into the new block
+and free the old block.
 If there is insufficient memory to allocate the new block,
-.B XtRealloc
-calls
+or the calculations for the size of the new block would cause an
+integer overflow, these functions call
 .BR XtErrorMsg .
 If ptr is NULL,
-.B XtRealloc
-allocates the new storage without copying the old contents;
-that is, it simply calls
+these functions allocate the new storage without copying the old contents;
+that is, they simply call
 .BR XtMalloc .
 .LP
 The
diff --git a/src/Alloc.c b/src/Alloc.c
index ce0594c..5bb27ce 100644
--- a/src/Alloc.c
+++ b/src/Alloc.c
@@ -1,5 +1,5 @@
 /***********************************************************
-Copyright (c) 1993, 2011, Oracle and/or its affiliates.
+Copyright (c) 1993, 2023, Oracle and/or its affiliates.
 
 Permission is hereby granted, free of charge, to any person obtaining a
 copy of this software and associated documentation files (the "Software"),
@@ -84,6 +84,7 @@ in this Software without prior written authorization from The Open Group.
 #include <stdlib.h>
 #include <stdio.h>
 #include <stdarg.h>
+#include <limits.h>
 
 #define Xmalloc(size) malloc((size))
 #define Xrealloc(ptr, size) realloc((ptr), (size))
@@ -197,6 +198,38 @@ XtRealloc(char *ptr, unsigned size)
     return (ptr);
 }
 
+void *
+XtReallocArray(void *ptr, unsigned num, unsigned size)
+{
+    if (ptr == NULL) {
+#ifdef MALLOC_0_RETURNS_NULL
+        if ((num == 0) || (size == 0))
+            num = size = 1;
+#endif
+#if (SIZE_MAX / UINT_MAX) <= UINT_MAX
+        if ((num > 0) && (SIZE_MAX / num) < size)
+            _XtAllocError("reallocarray: overflow detected");
+#endif
+        return (XtMalloc(num * size));
+    }
+    else {
+        void *new;
+
+#ifdef HAVE_REALLOCARRAY
+        new = reallocarray(ptr, size, num);
+#else
+# if (SIZE_MAX / UINT_MAX) <= UINT_MAX
+        if ((num > 0) && ((SIZE_MAX / num) < size))
+            _XtAllocError("reallocarray: overflow detected");
+# endif
+        new = Xrealloc(ptr, size * num);
+#endif
+        if ((new == NULL) && (num != 0) && (size != 0))
+            _XtAllocError("reallocarray");
+        return (new);
+    }
+}
+
 char *
 XtCalloc(unsigned num, unsigned size)
 {
@@ -402,6 +435,23 @@ XtRealloc(char *ptr, unsigned size)
     return _XtRealloc(ptr, size, (char *) NULL, 0);
 }
 
+void *
+_XtReallocArray(void *ptr, unsigned num, unsigned size, const char *file, int line)
+{
+#if (SIZE_MAX / UINT_MAX) <= UINT_MAX
+    if ((num > 0) && (SIZE_MAX / num) < size)
+        _XtAllocError("reallocarray: overflow");
+#endif
+
+    return _XtRealloc(ptr, (num * size), file, line);
+}
+
+void *
+XtReallocArray(void *ptr, unsigned num, unsigned size)
+{
+    return _XtReallocArray(ptr, num, size, (char *) NULL, 0);
+}
+
 char *
 _XtCalloc(unsigned num, unsigned size, const char *file, int line)
 {
diff --git a/test/Alloc.c b/test/Alloc.c
index 3120f0f..03f44a8 100644
--- a/test/Alloc.c
+++ b/test/Alloc.c
@@ -569,6 +569,166 @@ static void test_XtRealloc_overflow(void)
 }
 
 
+/* Make sure XtReallocArray() works for a non-zero amount of memory */
+static void test_XtReallocArray_normal(void)
+{
+    void *p, *p2;
+    char *p3;
+
+    errno = 0;
+
+    /* Make sure reallocarray with a NULL pointer acts as malloc */
+    p = XtReallocArray(NULL, 8, 14);
+    g_assert_nonnull(p);
+    CHECK_ALIGNMENT(p);
+    CHECK_SIZE(p, 8 * 14);
+
+    /* make sure we can write to all the allocated memory */
+    memset(p, 'A', 8 * 14);
+
+    /* create another block after the first */
+    p2 = XtMalloc(73);
+    g_assert_nonnull(p2);
+
+    /* now resize the first */
+    p3 = XtReallocArray(p, 73, 14);
+    g_assert_nonnull(p3);
+    CHECK_ALIGNMENT(p3);
+    CHECK_SIZE(p3, 73 * 14);
+    /* verify previous values are still present */
+    for (int i = 0; i < (8 * 14); i++) {
+        g_assert_cmpint(p3[i], ==, 'A');
+    }
+
+    XtFree(p3);
+    XtFree(p2);
+    g_assert_cmpint(errno, ==, 0);
+}
+
+/* Make sure XtReallocArray(0) returns a valid pointer as expected */
+static void test_XtReallocArray_zero(void)
+{
+    void *p, *p2;
+
+    errno = 0;
+
+    p = XtReallocArray(NULL, 0, 0);
+    g_assert_nonnull(p);
+
+    p2 = XtReallocArray(p, 0, 0);
+#ifdef MALLOC_0_RETURNS_NULL
+    g_assert_null(p);
+#else
+    g_assert_nonnull(p);
+#endif
+
+    XtFree(p2);
+    g_assert_cmpint(errno, ==, 0);
+}
+
+/* Make sure sizes larger than the limit we set in main() fail */
+static void test_XtReallocArray_oversize(void)
+{
+    void *p, *p2;
+
+    /* Pick a number of elements between 1 & 16K */
+    guint32 num = g_test_rand_int_range(1, (16 * 1024));
+    /* Pick a size between 1 & 16K */
+    guint32 size = g_test_rand_int_range(1, (16 * 1024));
+
+    p = XtReallocArray(NULL, num, size);
+    g_assert_nonnull(p);
+    CHECK_ALIGNMENT(p);
+    CHECK_SIZE(p, num * size);
+
+    if (setjmp(jmp_env) == 0) {
+        p2 = XtReallocArray(p, 2, ALLOC_LIMIT);
+        g_assert_null(p2);
+    } else {
+        /*
+         * We jumped here from error handler as expected.
+         * We cannot verify errno here, as the Xt error handler makes
+         * calls that override errno, when trying to load error message
+         * files from different locations.
+         */
+    }
+
+    errno = 0;
+    XtFree(p);
+    g_assert_cmpint(errno, ==, 0);
+}
+
+/* Make sure XtReallocArray catches integer overflow if possible, by requesting
+ * sizes that are so large that they cause overflows when either adding the
+ * reallocarray data block overhead or aligning.
+ *
+ * Testing integer overflow cases is limited by the fact that XtReallocArray
+ * only takes unsigned arguments (typically 32-bit), and relies on
+ * the underlying libc reallocarray to catch overflow, which can't occur if
+ * 32-bit arguments are passed to a function taking 64-bit args.
+ */
+static void test_XtReallocArray_overflow(void)
+{
+#if UINT_MAX < SIZE_MAX
+    g_test_skip("overflow not possible in this config");
+#else
+    void *p, *p2;
+
+    /* Pick a number of elements between 1 & 16K */
+    guint32 num = g_test_rand_int_range(1, (16 * 1024));
+    /* Pick a size between 1 & 16K */
+    guint32 size = g_test_rand_int_range(1, (16 * 1024));
+
+    p = XtReallocArray(NULL, num, size);
+    g_assert_nonnull(p);
+    CHECK_ALIGNMENT(p);
+    CHECK_SIZE(p, num * size);
+
+    if (setjmp(jmp_env) == 0) {
+        p2 = XtReallocArray(p, 1, SIZE_MAX);
+        g_assert_null(p2);
+    } else {
+        /*
+         * We jumped here from error handler as expected.
+         * We cannot verify errno here, as the Xt error handler makes
+         * calls that override errno, when trying to load error message
+         * files from different locations.
+         */
+    }
+
+    if (setjmp(jmp_env) == 0) {
+        /* SQRT_SIZE_MAX * SQRT_SIZE_MAX == 0 due to overflow */
+        p2 = XtReallocArray(p, SQRT_SIZE_MAX, SQRT_SIZE_MAX);
+        g_assert_null(p2);
+    } else {
+        /*
+         * We jumped here from error handler as expected.
+         * We cannot verify errno here, as the Xt error handler makes
+         * calls that override errno, when trying to load error message
+         * files from different locations.
+         */
+    }
+
+    if (setjmp(jmp_env) == 0) {
+        /* Overflows to a small positive number */
+        p2 = XtReallocArray(p, SQRT_SIZE_MAX + 1, SQRT_SIZE_MAX);
+        g_assert_null(p2);
+    } else {
+        /*
+         * We jumped here from error handler as expected.
+         * We cannot verify errno here, as the Xt error handler makes
+         * calls that override errno, when trying to load error message
+         * files from different locations.
+         */
+    }
+
+    errno = 0;
+    XtFree(p);
+    g_assert_cmpint(errno, ==, 0);
+#endif
+}
+
+
 int main(int argc, char** argv)
 {
     struct rlimit lim;
@@ -606,5 +766,12 @@ int main(int argc, char** argv)
     g_test_add_func("/Alloc/XtRealloc/oversize", test_XtRealloc_oversize);
     g_test_add_func("/Alloc/XtRealloc/overflow", test_XtRealloc_overflow);
 
+    g_test_add_func("/Alloc/XtReallocArray/normal", test_XtReallocArray_normal);
+    g_test_add_func("/Alloc/XtReallocArray/zero", test_XtReallocArray_zero);
+    g_test_add_func("/Alloc/XtReallocArray/oversize",
+                    test_XtReallocArray_oversize);
+    g_test_add_func("/Alloc/XtReallocArray/overflow",
+                    test_XtReallocArray_overflow);
+
     return g_test_run();
 }
diff --git a/test/Makefile.am b/test/Makefile.am
index adc7060..00229fb 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -5,7 +5,8 @@ check_PROGRAMS = Alloc Converters Event
 TESTS=$(check_PROGRAMS)
 
 AM_CFLAGS = $(CWARNFLAGS) $(XT_CFLAGS) $(GLIB_CFLAGS)
-AM_CPPFLAGS = -I$(top_srcdir)/include -I$(top_builddir)/include
+AM_CPPFLAGS = -I$(top_srcdir)/include -I$(top_builddir)/include \
+    -I$(top_builddir)/include/X11
 LDADD= $(top_builddir)/src/libXt.la $(GLIB_LIBS)
 
 TESTS_ENVIRONMENT = $(MALLOC_DEBUG_ENV)