Hi Paul,
Paul Eggert wrote:
> PS. The name END_OF_ARRAY is not the best, since a null pointer does not
> point to an array, and even when the pointer is non-null the macro can
> be used to point elsewhere than to the end of an array. A better name
> would be something like PTR_ADD, since the first arg is the pointer, the
> second arg the amount to add.
Good point. In the new proposed patch (attached), the macro is called PTR_ADD.
> Thanks, but I'm of two minds about that sort of thing. On the one hand,
> they document the assumption that (char *) NULL + 0 == NULL. On the
> other, they are still clutter
I see it differently. We should abandon the thinking that adding
(char *) NULL + 0
is well-defined. It was probably well-defined in the past, but is not
any more. The standard and its common interpretation are no more like
it was 20 years ago. Together with the standard, the tools change and
will change. The clang UBSAN has changed. GCC's UBSAN may change as well.
Compilers and static analysis tools may follow suit (i.e. give warnings
like "pointer operation on ptr, but ptr may be null").
It's like with other changes we saw in the past:
- strict aliasing rules - forbid to cast (int**) to (char**) etc.
- signed integer overflow.
What was once a common style is now outlawed.
Gnulib should not contain outlawed code; otherwise it becomes a bad
citizen that people (rightfully) complain about.
> and this assumption is safe on all but
> pedantic platforms -- and we've long not worried about pedantic
> platforms (e.g., gcc -pedantic), for good reason.
My point is not about pedantic platforms. I don't care about 'gcc -pedantic'
since it produces so many pointless diagnostics.
What I want is to be able to use (and that people are able to use)
clang + ASAN + UBSAN as a general compilation environment, since it
provides good runtime checking, better than CHERI and valgrind,
with zero false positives. And the
(char *) NULL + 0
is the _last_ obstacle; it hinders us from achieving this goal.
> Of course, the fact that you found these issues with clang's runtime
> checking does not mean that these are all the issues; I fully expect
> that there are others.
If there are some issues left, there will not be many. By running
clang + UBSAN I found only two issues:
- obstack,
- (char *) NULL + 0
> All in all, it may be better to leave the code alone.
I disagree. The goals of
- making full use of UBSAN,
- avoid future compiler warnings as compilers get stricter on this issue
are more important than one (one!) additional macro that is used in very few
places. The maintenance cost of having one additional macro is near zero.
Bruno
From ce227dca145176a17342a52a798459fcb37e0bfd Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Sat, 16 Dec 2023 21:40:33 +0100
Subject: [PATCH] linebuffer, mpsort: Avoid undefined behaviour adding pointer
+ integer.
* lib/pointer-op.h: New file.
* lib/linebuffer.c: Include pointer-op.h.
(readlinebuffer_delim): Use PTR_ADD.
* lib/mpsort.c: Include pointer-op.h.
(mpsort): Use PTR_ADD.
* modules/linebuffer (Files): Add lib/pointer-op.h.
* modules/mpsort (Files): Likewise.
---
ChangeLog | 11 +++++++++++
lib/linebuffer.c | 8 ++++++--
lib/mpsort.c | 5 ++++-
lib/pointer-op.h | 40 ++++++++++++++++++++++++++++++++++++++++
modules/linebuffer | 1 +
modules/mpsort | 1 +
6 files changed, 63 insertions(+), 3 deletions(-)
create mode 100644 lib/pointer-op.h
diff --git a/ChangeLog b/ChangeLog
index 13e80727ba..06ac734286 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2023-12-16 Bruno Haible <br...@clisp.org>
+
+ linebuffer, mpsort: Avoid undefined behaviour adding pointer + integer.
+ * lib/pointer-op.h: New file.
+ * lib/linebuffer.c: Include pointer-op.h.
+ (readlinebuffer_delim): Use PTR_ADD.
+ * lib/mpsort.c: Include pointer-op.h.
+ (mpsort): Use PTR_ADD.
+ * modules/linebuffer (Files): Add lib/pointer-op.h.
+ * modules/mpsort (Files): Likewise.
+
2023-12-14 Paul Eggert <egg...@cs.ucla.edu>
mcel-tests: fix thinko in test
diff --git a/lib/linebuffer.c b/lib/linebuffer.c
index 4124bbcb37..19b7325a52 100644
--- a/lib/linebuffer.c
+++ b/lib/linebuffer.c
@@ -20,11 +20,15 @@
#include <config.h>
+/* Specification. */
+#include "linebuffer.h"
+
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
-#include "linebuffer.h"
+
+#include "pointer-op.h"
#include "xalloc.h"
#if USE_UNLOCKED_IO
@@ -62,7 +66,7 @@ readlinebuffer_delim (struct linebuffer *linebuffer, FILE *stream,
int c;
char *buffer = linebuffer->buffer;
char *p = linebuffer->buffer;
- char *end = buffer + linebuffer->size; /* Sentinel. */
+ char *end = PTR_ADD (buffer, linebuffer->size); /* Sentinel. */
if (feof (stream))
return NULL;
diff --git a/lib/mpsort.c b/lib/mpsort.c
index 32cc5e1f82..4e0c3ac70f 100644
--- a/lib/mpsort.c
+++ b/lib/mpsort.c
@@ -19,10 +19,13 @@
#include <config.h>
+/* Specification. */
#include "mpsort.h"
#include <string.h>
+#include "pointer-op.h"
+
/* The type of qsort-style comparison functions. */
typedef int (*comparison_function) (void const *, void const *);
@@ -152,5 +155,5 @@ mpsort_with_tmp (void const **restrict base, size_t n,
void
mpsort (void const **base, size_t n, comparison_function cmp)
{
- mpsort_with_tmp (base, n, base + n, cmp);
+ mpsort_with_tmp (base, n, PTR_ADD (base, n), cmp);
}
diff --git a/lib/pointer-op.h b/lib/pointer-op.h
new file mode 100644
index 0000000000..4167502a66
--- /dev/null
+++ b/lib/pointer-op.h
@@ -0,0 +1,40 @@
+/* Pointer operations.
+ Copyright (C) 2023 Free Software Foundation, Inc.
+
+ This file is free software: you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as
+ published by the Free Software Foundation; either version 2.1 of the
+ License, or (at your option) any later version.
+
+ This file is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with this program. If not, see <https://www.gnu.org/licenses/>. */
+
+#ifndef _POINTER_OP_H
+#define _POINTER_OP_H
+
+/* PTR_ADD(POINTER,N) returns a generalization of the ISO C definition of
+ POINTER + N.
+ Namely, when POINTER + N is well-defined according to ISO C 23 § 6.5.6.(9),
+ this macro returns it. Otherwise, when N is zero, it merely returns the
+ POINTER.
+
+ If POINTER points to an array of N elements, this macro returns a pointer
+ past the last element of the array.
+
+ For clang, it uses a conditional expression that avoids adding 0 to a null
+ pointer, which is undefined according to ISO C 23 § 6.5.6.(9) and which
+ triggers an error in clang's undefined-behaviour sanitizer. When no
+ sanitizer is in effect, clang optimizes this conditional expression to
+ exactly the same code. */
+#if defined __clang__
+# define PTR_ADD(pointer,n) ((n) ? (pointer) + (n) : (pointer))
+#else
+# define PTR_ADD(pointer,n) ((pointer) + (n))
+#endif
+
+#endif /* _POINTER_OP_H */
diff --git a/modules/linebuffer b/modules/linebuffer
index 4ddd3c38ed..d142eeee7b 100644
--- a/modules/linebuffer
+++ b/modules/linebuffer
@@ -4,6 +4,7 @@ Read a line from a stream.
Files:
lib/linebuffer.h
lib/linebuffer.c
+lib/pointer-op.h
Depends-on:
idx
diff --git a/modules/mpsort b/modules/mpsort
index 054e979c2e..aef0432ce3 100644
--- a/modules/mpsort
+++ b/modules/mpsort
@@ -4,6 +4,7 @@ Sort a vector of pointers to data.
Files:
lib/mpsort.h
lib/mpsort.c
+lib/pointer-op.h
m4/mpsort.m4
Depends-on:
--
2.34.1
