> > 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.

I added "-fsanitize=undefined" when testing. Sorry I haven't explained that.
With the “-fsanitize=undefined” option enabled, I'm using GCC 14 and Clang
16. And both were able to reproduce the issue.
I demonstrated the error using my own github repository below.

  https://github.com/nak3/portable/pull/1

The error occurs in bio_mem. (Although "make test" doesn’t show the
detailed error message...)

> 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.

Yes, exactly. I confirmed that it only occurs in the first test case (no
data).

> 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.

You’re absolutely right - it was also strange to me.
With that said, I guessed that strncmp(*str1, *str2, num) might be
implemented in such a way that it doesn't access the pointer when the third
argument (=num) is zero.
Or, it's possible that the sanitizer doesn't detect issues inside the libc
implementation.

> 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;
>         }


Thank you.
You’re right. My fix does add the check even when it isn't needed.
I reviewed your fix as well, and I think it’s a good solution. I also
confirmed that it resolves the sanitizer error in my environment.

Regards,
Kenjiro

On Sun, Apr 13, 2025 at 6:49 PM Theo Buehler <tb@theobuehler.org> wrote:

> 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;
>         }
>


-- 
Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com>
GPG Key fingerprint = ED8F 049D E67A 727D 9A44  8E25 F44B E208 C946 5EB9