Bug 283698 - New version of swab(3) breaks port graphics/dcraw
Summary: New version of swab(3) breaks port graphics/dcraw
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: 15.0-CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Warner Losh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-12-28 20:19 UTC by Nathan Whitehorn
Modified: 2025-01-07 15:07 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nathan Whitehorn freebsd_committer freebsd_triage 2024-12-28 20:19:13 UTC
The new version of swab(3) committed in bac2eea13ae3e4dc8fd2aec261b2a18930138495 has slightly different semantics than the old version. In particular, if the input and output buffers are identical, the old version will work, but the new one -- which does not use a temporary buffer like the old one did -- will not.

Strictly speaking, this is fine since the input and output pointers are marked 'restrict' so cannot be identical, but at least one program in the wild (graphics/dcraw and all the versions of it in other photography programs) uses identical buffers anyway and expects it to work.

Closing this "won't fix" is legitimate -- I will alert the dcraw developers as well, since the bug is clearly on their end -- but I wanted to also write something here since there appear to be compatibility implications both relative to Linux and older versions of FreeBSD.
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2024-12-29 05:41:48 UTC
^Triage: over to committer of change in question.
Comment 2 Warner Losh freebsd_committer freebsd_triage 2024-12-29 06:07:25 UTC
It would be easy enough to use the old step method.

But the code that calls it so would fail on musl. Ans swab(p,p) relies on the compiler not doing stupid stuff it may have a license to do given the restrict keyword...
Comment 3 Nathan Whitehorn freebsd_committer freebsd_triage 2024-12-30 13:20:26 UTC
(In reply to Warner Losh from comment #2)

Yeah, exactly -- I'm not sure what the best path is. I found similar reports of problems with MUSL's implementation (e.g. https://www.openwall.com/lists/musl/2022/12/28/1 and someone complaining on Reddit about a MUSL Gentoo system + rawtherapee, which is also affected, that might be the same issue), and MUSL decided not to change their implementation. I think the number of people doing GUI photography on MUSL might be even lower than the number of people doing it on FreeBSD, though.
Comment 4 Warner Losh freebsd_committer freebsd_triage 2024-12-30 15:45:33 UTC
While POSIX does say "If copying takes place between objects that overlap, the behavior is undefined." in the description of swab, it's easy enough to make overlapping objects work. I think I'll post a diff to make that happen, though it will make things slightly more complicated. I doubt there will be any speed difference.
Comment 5 Warner Losh freebsd_committer freebsd_triage 2024-12-30 19:06:05 UTC
We also have a x86 .S version that handles overlap as expected...
Comment 6 Warner Losh freebsd_committer freebsd_triage 2024-12-30 20:39:26 UTC
https://reviews.freebsd.org/D48259
Comment 7 commit-hook freebsd_committer freebsd_triage 2025-01-06 23:47:25 UTC
A commit in branch main references this bug:

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

commit 02ebbc781f082df9714e74775700d8c08bac7850
Author:     Warner Losh <imp@FreeBSD.org>
AuthorDate: 2025-01-06 23:44:21 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2025-01-06 23:46:05 +0000

    swab: Fix implementation to support overlapping copies

    A number of image processing packages assume that swab() can handle to
    and from being the same. However, POSIX.1 states that overlapping
    buffers produces undefined results. Our old implementation would produce
    coherent results, but the recent change to the musl-inspired code does
    not. Since there's complaints in the forums for these image processing
    packages for musl and now FreeBSD, update the algorithm to just read a
    word at a time and bswap16 the results. All FreeBSD's architecutres
    support unaligned access in userland, and swab is not used in the kernel
    (g_part_apm has its own copy), so opt for even simpler code that's
    easier to understand. This makes the overlapping behavior match i386 again,
    since its assembler routine for swab handles overlapping correctly.

    PR: 283698
    Sponsored by: Netflix
    Reviewed by:    nwhitehorn
    Differential Revision:  https://reviews.freebsd.org/D48259

 lib/libc/string/swab.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)