Bug 273652 - libc: broken memchr(3) after base de12a689fad2
Summary: libc: broken memchr(3) after base de12a689fad2
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.0-CURRENT
Hardware: amd64 Any
: --- Affects Some People
Assignee: Robert Clausecker
URL: https://cgit.freebsd.org/src/commit/?...
Keywords: regression
Depends on:
Blocks:
 
Reported: 2023-09-09 07:27 UTC by (intentionally left blank)
Modified: 2023-09-23 18:22 UTC (History)
7 users (show)

See Also:


Attachments
lib/libc/amd64/string/memchr.S: fix behaviour with overly long buffers (1.88 KB, patch)
2023-09-10 04:42 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 (intentionally left blank) 2023-09-09 07:27:26 UTC
textproc/jq will fail to configure until base de12a689fad2 is reverted:

[...]
sed: No error: 0
checking for sys/cygwin.h... eval: ${+...}: Bad substitution
[...]

(I additionally reverted parent commit base 331737281c19).
Comment 1 Robert Clausecker freebsd_committer freebsd_triage 2023-09-09 15:37:52 UTC
Thank you for the report.  This sucks!  Let's find out what the problem is, shall we?

Could you re-run the build with the patch in and ARCHLEVEL=scalar in the environment?  This uses a non-SIMD implementation of memchr().  If you could find out which command specifically fails here, that would be great, too.

My development machine for the SIMD stuff is currently offline due to operator error but will come back online soon, so I can personally investigate.
Comment 2 Antoine Brodin freebsd_committer freebsd_triage 2023-09-09 18:57:33 UTC
(In reply to Robert Clausecker from comment #1)
With ARCHLEVEL=scalar ,  configure succeeds.
Comment 3 Robert Clausecker freebsd_committer freebsd_triage 2023-09-09 22:13:09 UTC
Thanks.  I will investigate this.
Comment 4 John F. Carr 2023-09-09 23:22:37 UTC
Simpler reproduction:

env -i /usr/bin/paste -s -d , -

fails on latest 15 apparently due to mbsrtowcs failing, waits for input on 13.2
Comment 5 John F. Carr 2023-09-09 23:34:16 UTC
I may have found a different problem because the one I found is not affected by ARCHLEVEL:

# env -i ARCHLEVEL=scalar LANG=C.UTF-8 /usr/bin/paste -s -d , /dev/null
# env -i ARCHLEVEL=baseline LANG=C.UTF-8 /usr/bin/paste -s -d , /dev/null
# env -i ARCHLEVEL=baseline LANG=C /usr/bin/paste -s -d , /dev/null
paste: delimiters: No error: 0
# env -i ARCHLEVEL=scalar LANG=C /usr/bin/paste -s -d , /dev/null
paste: delimiters: No error: 0
# env -i ARCHLEVEL=scalar LANG=foobar /usr/bin/paste -s -d , /dev/null
paste: delimiters: Invalid argument
Comment 6 Kyle Evans freebsd_committer freebsd_triage 2023-09-10 01:53:07 UTC
(In reply to John F. Carr from comment #5)

The only place I can see this failure mode happening (-1 return, no errno set) is here:

https://cgit.freebsd.org/src/tree/lib/libc/locale/none.c#n139


The two other return paths use nchr, which starts at 0 and is only ever incremented.
Comment 7 Robert Clausecker freebsd_committer freebsd_triage 2023-09-10 04:42:24 UTC
Created attachment 244743 [details]
lib/libc/amd64/string/memchr.S: fix behaviour with overly long buffers

This should fix the issue.

The root cause seem to be calls to memchr(buf, c, SIZE_MAX) which I did not account for when writing the code.  I assumed the length would always be valid
and computed end = buf + len without accounting for possible overflow.  This
change changes the computation such that end will be set to (void *)UINTPTR_MAX
if buf + len overflows.

Please check and let me know if this fixes the issues you observed.
memcmp() is likely affected by the same issue, though calling memcmp() with a
phony buffer length is undefined behaviour, so I don't think it should affect
correct code much.
Comment 8 (intentionally left blank) 2023-09-10 06:12:53 UTC
(In reply to Robert Clausecker from comment #7)

I confirm, poudriere testport textproc/jq runs fine here with patch applied.

Off-topic: please either don't credit me (preferred), or credit as anonymous.
Comment 9 commit-hook freebsd_committer freebsd_triage 2023-09-10 12:59:57 UTC
A commit in branch main references this bug:

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

commit b2618b651b28fd29e62a4e285f5be09ea30a85d4
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2023-09-10 04:11:07 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2023-09-10 12:52:59 +0000

    lib/libc/amd64/string/memchr.S: fix behaviour with overly long buffers

    When memchr(buf, c, len) is called with a phony len (say, SIZE_MAX),
    buf + len overflows and we have buf + len < buf.  This confuses the
    implementation and makes it return incorrect results.  Neverthless we
    must support this case as memchr() is guaranteed to work even with
    phony buffer lengths, as long as a match is found before the buffer
    actually ends.

    Sponsored by:   The FreeBSD Foundation
    Reported by:    yuri, des
    Tested by:      des
    Approved by:    mjg (blanket, via IRC)
    MFC after:      1 week
    MFC to:         stable/14
    PR:             273652

 lib/libc/amd64/string/memchr.S | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Comment 10 Robert Clausecker freebsd_committer freebsd_triage 2023-09-10 13:00:26 UTC
Thank you for the report!
Comment 11 commit-hook freebsd_committer freebsd_triage 2023-09-23 18:22:44 UTC
A commit in branch stable/14 references this bug:

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

commit 3f78bde932d31e8c80e3201653abae96f8c5f45b
Author:     Robert Clausecker <fuz@FreeBSD.org>
AuthorDate: 2023-08-24 16:43:40 +0000
Commit:     Robert Clausecker <fuz@FreeBSD.org>
CommitDate: 2023-09-23 18:20:28 +0000

    lib/libc/amd64/string: add memchr(3) scalar, baseline implementation

    This is conceptually similar to strchr(3), but there are
    slight changes to account for the buffer having an explicit
    buffer length.

    this includes the bug fix from b2618b6.

    Sponsored by:   The FreeBSD Foundation
    Reported by:    yuri, des
    Tested by:      des
    Approved by:    mjg
    MFC after:      1 week
    MFC to:         stable/14
    PR:             273652
    Differential Revision:  https://reviews.freebsd.org/D41598

    (cherry picked from commit de12a689fad271f5a2ba7c188b0b5fb5cabf48e7)
    (cherry picked from commit b2618b651b28fd29e62a4e285f5be09ea30a85d4)

 lib/libc/amd64/string/Makefile.inc   |   1 +
 lib/libc/amd64/string/memchr.S (new) | 207 +++++++++++++++++++++++++++++++++++
 2 files changed, 208 insertions(+)