Bug 293915 - strrchr(3) has bug for amd64 ARCHLEVEL=scalar
Summary: strrchr(3) has bug for amd64 ARCHLEVEL=scalar
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: standards (show other bugs)
Version: 16.0-CURRENT
Hardware: amd64 Any
: --- Affects Many People
Assignee: Robert Clausecker
URL: https://reviews.freebsd.org/D56037
Keywords:
Depends on:
Blocks:
 
Reported: 2026-03-19 08:32 UTC by safonov.paul
Modified: 2026-04-08 13:22 UTC (History)
2 users (show)

See Also:


Attachments
libc/amd64/strrchr.S: rewrite scalar kernel (5.68 KB, patch)
2026-03-22 14:00 UTC, Robert Clausecker
no flags Details | Diff
libc/amd64/strrchr.S: rewrite scalar kernel (5.07 KB, patch)
2026-03-23 13:09 UTC, Robert Clausecker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description safonov.paul 2026-03-19 08:32:11 UTC
> uname -mrU
14.4-STABLE amd64 1404500

> cat s.c
#include <stdio.h>
#include <string.h>

int main(void)
{
        char    t[] = "/tmp";
        char    *p = strrchr(t, '\0');

        printf("*** &t = %p, p = %p\n", &t, p);

        return 0;
}

> cc -o s s.c

> ARCHLEVEL=scalar ./s
*** &t = 0x820339157, p = 0x82033915f


The function strrchr(3) returns a pointer outside the buffer for the ARCHLEVEL=scalar mode.
Comment 1 fakeshadow1337@gmail.com 2026-03-19 12:34:52 UTC
The bug is rooted in lib/libc/amd64/string/strrchr.S. In the scalar implementation, the bit-parallel optimization—specifically the bswap logic combined with unaligned memory masks—fails to correctly calculate the offset when the target character is the NUL terminator. I have verified that intercepting the NUL character at the function entry and redirecting it to a safe scalar scan path effectively eliminates this 8-byte address deviation.
Comment 2 Robert Clausecker freebsd_committer freebsd_triage 2026-03-19 19:46:07 UTC
Thank you for the report.  Please let me investigate.
Comment 3 Robert Clausecker freebsd_committer freebsd_triage 2026-03-22 14:00:13 UTC
Created attachment 268998 [details]
libc/amd64/strrchr.S: rewrite scalar kernel

Please find attached a preliminary patch for this issue.
I am not quite happy with the new logic and will probably
rework it further.

Please let me know if it works for you.

The bug was likely not caught due to the insufficient test
suite inherited from the NetBSD project.  I will add further
unit tests so that this sort of issue will be caught in the
future (e.g. on other platforms).
Comment 4 Robert Clausecker freebsd_committer freebsd_triage 2026-03-22 21:42:52 UTC
New unit test that catches the issue you reported:

    https://reviews.freebsd.org/D56037

Unfortunately, the patch in attachment #268998 [details] fails this one, too.
It passes more cases than the current code though.
Comment 5 Robert Clausecker freebsd_committer freebsd_triage 2026-03-23 13:09:47 UTC
Created attachment 269044 [details]
libc/amd64/strrchr.S: rewrite scalar kernel

Please find attached a revised patch that actually passes the unit tests proposed in D56037.
Comment 6 Robert Clausecker freebsd_committer freebsd_triage 2026-03-25 13:11:04 UTC
(In reply to shadowing from comment #1)

If you have some time, I would appreciate if you could test the patch (attachment #269044 [details]) before I land it.
Comment 7 fakeshadow1337@gmail.com 2026-03-26 08:47:26 UTC
(In reply to Robert Clausecker from comment #6)

It works fine for me, on the `FreeBSD 15.0-RELEASE GENERIC amd64` platform. All unit tests passed.
Comment 8 commit-hook freebsd_committer freebsd_triage 2026-03-26 11:42:18 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=253f15c016ca699906f78b8e522a3f7ed675929b

commit 253f15c016ca699906f78b8e522a3f7ed675929b
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2026-03-22 12:37:06 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2026-03-26 11:41:49 +0000

    libc/amd64/strrchr.S: rewrite and fix scalar implementation

    The original scalar implementation of strrchr() had incorrect
    logic that failed if the character searched for was the NUL
    character.  It was also possibly affected by the issue fixed
    in 3d8ef251a for strchrnul().

    Rewrite the function with logic that actually works.  We defer
    checking for the character until after we have checked for NUL.
    When we encounter the final NUL byte, we mask out the characters
    beyond the tail before checking for a match.

    This bug only affects users running on amd64 with ARCHLEVEL=scalar
    (cf. simd(7)).  The default configuration is not affected.

    The bug was unfortunately not caught by the unit test inherited
    from NetBSD.  An extended unit test catching the issue is proposed
    in D56037.

    PR:             293915
    Reported by:    safonov.paul@gmail.com
    Tested by:      safonov.paul@gmail.com
    Fixes:          2ed514a220edbac6ca5ec9f40a3e0b3f2804796d
    See also:       https://reviews.freebsd.org/D56037
    MFC after:      1 week

 lib/libc/amd64/string/strrchr.S | 78 ++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 52 deletions(-)
Comment 9 commit-hook freebsd_committer freebsd_triage 2026-03-26 13:03:28 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=23d6516773916d8f324bea51867b0713c476f379

commit 23d6516773916d8f324bea51867b0713c476f379
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2026-03-26 13:00:21 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2026-03-26 13:01:57 +0000

    libc/amd64/strrchr.S: fix rebase error

    I accidentally dropped a part of the patch on squash rebase.
    Should be fine now.

    Fixes:          253f15c016ca699906f78b8e522a3f7ed675929b
    PR:             293915
    MFC after:      1 week

 lib/libc/amd64/string/strrchr.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Comment 10 fakeshadow1337@gmail.com 2026-03-26 13:10:01 UTC
(In reply to commit-hook from comment #8)

I noticed that the `Tested by` tag currently lists the reporter's email. I think it might be a mistake, since it was me who tested it.
Comment 11 Robert Clausecker freebsd_committer freebsd_triage 2026-03-26 13:50:09 UTC
(In reply to fakeshadow1337@gmail.com from comment #10)

Indeed!  Sorry, I missed that it was not the submitter who tested.
In any case, the patch you tested was defective (I missed a hunk in the squash-rebase), so I'm not sure what you actually tested, but it shouldn't have worked.
Likely you missed to add ARCHLEVEL=scalar during testing.
Comment 12 fakeshadow1337@gmail.com 2026-03-26 14:32:22 UTC
(In reply to Robert Clausecker from comment #11)

Hmm, I applied your new patch, here is the output:

`root@bsd:/usr/src/lib/libc # env LD_LIBRARY_PATH=/usr/obj/usr/src/amd64.amd64/lib/libc/ ARCHLEVEL=scalar ~/s
*** &t = 0x82036edd7, p = 0x7fffffffff
`

And the original patch is fine for this case.
Comment 13 Robert Clausecker freebsd_committer freebsd_triage 2026-03-26 14:43:09 UTC
(In reply to fakeshadow1337@gmail.com from comment #12)

Please also apply the hunk from base 23d6516773916d8f324bea51867b0713c476f379 to the patch from attachment #269044 [details].  As you can see, the match vector and pointer were accidentally swapped (I had fixed the swap in a fix up commit that I forgot to squash in).
Comment 14 fakeshadow1337@gmail.com 2026-03-26 15:00:12 UTC
(In reply to Robert Clausecker from comment #13)

I indeed applied the hunk from base 23d6516773916d8f324bea51867b0713c476f379 to the patch from attachment #269044 [details], and that's the output:

`
root@bsd:/usr/src/lib/libc # env LD_LIBRARY_PATH=/usr/obj/usr/src/amd64.amd64/lib/libc/ ARCHLEVEL=scalar ~/s
*** &t = 0x82036edd7, p = 0x7fffffffff
`

When I revert the change at @@ -104,8 +104,8 @@ ARCHENTRY(strrchr, scalar), or revert the whole commit of 23d6516773916d8f324bea51867b0713c476f379, the output is correct:

`root@bsd:/usr/src/lib/libc # env LD_LIBRARY_PATH=/usr/obj/usr/src/amd64.amd64/lib/libc/ ARCHLEVEL=scalar ~/s
*** &t = 0x820e56e67, p = 0x820e56e6b
`
Comment 15 fakeshadow1337@gmail.com 2026-03-26 15:26:49 UTC
(In reply to Robert Clausecker from comment #13)

My apologies, sorry. I found that I had made a mistake during the manual patching process—the previous "defective" output was due to my own incorrect application of the missing hunk.
Comment 16 commit-hook freebsd_committer freebsd_triage 2026-04-02 07:59:59 UTC
A commit in branch stable/15 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=81114ff7ebc1efbd65d071869836c76f6fded2b3

commit 81114ff7ebc1efbd65d071869836c76f6fded2b3
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2026-03-22 12:37:06 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2026-04-02 07:58:52 +0000

    libc/amd64/strrchr.S: rewrite and fix scalar implementation

    The original scalar implementation of strrchr() had incorrect
    logic that failed if the character searched for was the NUL
    character.  It was also possibly affected by the issue fixed
    in 3d8ef251a for strchrnul().

    Rewrite the function with logic that actually works.  We defer
    checking for the character until after we have checked for NUL.
    When we encounter the final NUL byte, we mask out the characters
    beyond the tail before checking for a match.

    This bug only affects users running on amd64 with ARCHLEVEL=scalar
    (cf. simd(7)).  The default configuration is not affected.

    The bug was unfortunately not caught by the unit test inherited
    from NetBSD.  An extended unit test catching the issue is proposed
    in D56037.

    PR:             293915
    Reported by:    safonov.paul@gmail.com
    Tested by:      safonov.paul@gmail.com
    Fixes:          2ed514a220edbac6ca5ec9f40a3e0b3f2804796d
    See also:       https://reviews.freebsd.org/D56037
    MFC after:      1 week

    (cherry picked from commit 253f15c016ca699906f78b8e522a3f7ed675929b)
    (cherry picked from commit 23d6516773916d8f324bea51867b0713c476f379)

 lib/libc/amd64/string/strrchr.S | 78 ++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 52 deletions(-)
Comment 17 commit-hook freebsd_committer freebsd_triage 2026-04-02 08:02:01 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ed71162cc527d7f6bf5bfe7258f5e1a5cdb4cc71

commit ed71162cc527d7f6bf5bfe7258f5e1a5cdb4cc71
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2026-03-22 12:37:06 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2026-04-02 08:00:47 +0000

    libc/amd64/strrchr.S: rewrite and fix scalar implementation

    The original scalar implementation of strrchr() had incorrect
    logic that failed if the character searched for was the NUL
    character.  It was also possibly affected by the issue fixed
    in 3d8ef251a for strchrnul().

    Rewrite the function with logic that actually works.  We defer
    checking for the character until after we have checked for NUL.
    When we encounter the final NUL byte, we mask out the characters
    beyond the tail before checking for a match.

    This bug only affects users running on amd64 with ARCHLEVEL=scalar
    (cf. simd(7)).  The default configuration is not affected.

    The bug was unfortunately not caught by the unit test inherited
    from NetBSD.  An extended unit test catching the issue is proposed
    in D56037.

    PR:             293915
    Reported by:    safonov.paul@gmail.com
    Tested by:      safonov.paul@gmail.com
    Fixes:          2ed514a220edbac6ca5ec9f40a3e0b3f2804796d
    See also:       https://reviews.freebsd.org/D56037
    MFC after:      1 week

    (cherry picked from commit 253f15c016ca699906f78b8e522a3f7ed675929b)
    (cherry picked from commit 23d6516773916d8f324bea51867b0713c476f379)

 lib/libc/amd64/string/strrchr.S | 78 ++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 52 deletions(-)
Comment 18 commit-hook freebsd_committer freebsd_triage 2026-04-08 13:22:45 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8b5d77bbcbd98e684226950be1c779e108059d8d

commit 8b5d77bbcbd98e684226950be1c779e108059d8d
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2026-03-22 21:39:42 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2026-04-08 13:21:50 +0000

    libc/tests/string: add a more comprehensive unit test for strrchr()

    The unit tests are patterned after those for memrchr().
    This catches the issue found in 293915.

    PR:             293915
    Reviewed by:    strajabot
    Reported by:    safonov.paul@gmail.com
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D56037

 lib/libc/tests/string/Makefile             |   2 +
 lib/libc/tests/string/strrchr_test.c (new) | 156 +++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+)