On Sat, Apr 12, 2025 at 12:15:42PM +0900, Kenjiro Nakayama wrote:
> When bm->buf->data is NULL, calling bio_mem_read_ptr() triggers a runtime
> error under UndefinedBehaviorSanitizer:

What version are you using? I couldn't reproduce with gcc 14 and clang 19.

> 
>     $ ./tests/bio_dump
>     /dev/portable/crypto/bio/bss_mem.c:87:10: runtime error: applying zero offset to null pointer
>     SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /dev/portable/crypto/bio/bss_mem.c:87:10 in
> 
> This patch adds an explicit NULL check to avoid applying an offset
> to a NULL pointer, which is undefined behavior. The function now
> safely returns NULL if the buffer is uninitialized.

I think bio_mem_read_ptr() should only be called if we know that there's
data available for reading. This is checked to be the case in all callers
with one exception.

This exception is triggered by the corner case I added to the bio_dump
test to check the output when no data is dumped. In this case the mem
bio does not set up a data buffer because nothing is written to it and
therefore the data pointer is NULL.

I'm confused why your fix is enough. It seems ubsan doesn't complain
about the strncmp(..., NULL, 0) that then follows, which I'm pretty
sure is also UB.

Anyway, while your fix would be one way of doing it, it also adds this
check when it isn't needed. I think it is better to do something like
this instead: to my mind the check for ->max is the most sensible one
here, but we could also check directly for ->data != NULL.

Index: lib/libcrypto/bio/bss_mem.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/bio/bss_mem.c,v
diff -u -p -r1.22 bss_mem.c
--- lib/libcrypto/bio/bss_mem.c	5 Jul 2023 21:23:37 -0000	1.22
+++ lib/libcrypto/bio/bss_mem.c	12 Apr 2025 17:05:15 -0000
@@ -283,7 +283,9 @@ mem_ctrl(BIO *bio, int cmd, long num, vo
 	case BIO_CTRL_INFO:
 		if (ptr != NULL) {
 			pptr = (void **)ptr;
-			*pptr = bio_mem_read_ptr(bm);
+			*pptr = NULL;
+			if (bm->buf->max > 0)
+				*pptr = bio_mem_read_ptr(bm);
 		}
 		ret = (long)bio_mem_pending(bm);
 		break;
Index: regress/lib/libcrypto/bio/bio_dump.c
===================================================================
RCS file: /cvs/src/regress/lib/libcrypto/bio/bio_dump.c,v
diff -u -p -r1.4 bio_dump.c
--- regress/lib/libcrypto/bio/bio_dump.c	9 Feb 2024 12:48:32 -0000	1.4
+++ regress/lib/libcrypto/bio/bio_dump.c	13 Apr 2025 09:35:54 -0000
@@ -809,7 +809,7 @@ bio_dump_test(const struct bio_dump_test
 		    tc->indent, ret, got_len, strlen(tc->output));
 		goto err;
 	}
-	if (strncmp(tc->output, got, got_len) != 0) {
+	if (got_len > 0 && strncmp(tc->output, got, got_len) != 0) {
 		fprintf(stderr, "%d: mismatch\n", tc->indent);
 		goto err;
 	}