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

Just to clarify. I meant bio_dump test - "FAIL: bio_dump".

On Mon, Apr 14, 2025 at 8:35 AM Nakayama Kenjiro <nakayamakenjiro@gmail.com>
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.
>
> 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
>


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