Page MenuHomeFreeBSD

Fix a few UBSan errors in bootstrap-tools
Needs ReviewPublic

Authored by arichardson on Jan 19 2021, 11:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 20 2023, 8:44 AM
Unknown Object (File)
Dec 11 2023, 6:20 AM
Unknown Object (File)
Sep 6 2023, 8:15 AM
Unknown Object (File)
Aug 14 2023, 11:12 PM
Unknown Object (File)
Aug 14 2023, 12:31 PM
Unknown Object (File)
Jul 21 2023, 2:32 PM
Unknown Object (File)
Apr 25 2023, 10:07 PM
Unknown Object (File)
Jan 19 2023, 2:07 AM
Subscribers

Details

Summary

This commit fixes the errors I encountered when performing a buildworld with
the bootstrap tools compiled with -fsanitize=undefined.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 36408
Build 33297: arc lint + arc unit

Event Timeline

contrib/elftoolchain/libdwarf/_libdwarf.h
40

stdint.h sorts before stdio.h.

contrib/elftoolchain/libdwarf/libdwarf_rw.c
423

Same problem here, no?

contrib/elftoolchain/libdwarf/libdwarf_rw.c
423

Yes, I guess that path just didn't get executed during buildworld. Will update patch.

contrib/elftoolchain/libdwarf/libdwarf_rw.c
441

And here.

usr.bin/sort/bwstring.h
74

This field is needed since otherwise the compiler complains: https://godbolt.org/z/TqeEcn
flexible array member 'wstr' not allowed in otherwise empty struct

LGTM with the modifications you proposed.

usr.bin/sort/bwstring.h
74

Would a nice but more invasive solution be to define two named structures containing wstr, and cstr, and make bwstring a union of the two?

This revision is now accepted and ready to land.Jan 21 2021, 3:01 PM
usr.bin/sort/bwstring.h
74

I would say so. There are "only" 57 instances of ->len (and no .len) so it wouldn't be _that_ painful to fix if someone felt sufficiently motivated.

usr.bin/sort/bwstring.h
74

I agree it would be much nicer, maybe it's not too difficult to update with a bit of search+replace

Avoid the nested union in sort. Split into 4 commits locally:

commit 68d1b92fc08efa56115f8873fd7e8240f2a05653 (HEAD -> crossbuild-cleaned)
Author: Alex Richardson <arichardson@FreeBSD.org>
Date:   Fri Jan 22 14:52:59 2021 +0000

    ctfconvert: Fix UBSan errors
    
    This commit fixes errors I encountered when performing a buildworld
    with the bootstrap tools compiled with -fsanitize=undefined.

commit af6e804e7b504279a48f9923c2fb6a15ddbed2b5
Author: Alex Richardson <arichardson@FreeBSD.org>
Date:   Fri Jan 22 14:52:04 2021 +0000

    libdwarf: Fix UBsan shift errors
    
    Fix these by casting to uint64_t and using UINT64_MAX instead of -1.

commit f898a97acc95917f1b59081c6e49d090e72d7eb8
Author: Alex Richardson <arichardson@FreeBSD.org>
Date:   Fri Jan 22 14:47:54 2021 +0000

    stdlib.h: mark ___mb_cur_max() with __pure
    
    This is called quite a lot by usr.bin/sort. Hopefully this will allow
    eliding some of those redundant calls.

commit 530391b70634110534cb98dc99e06f9a63fcf703
Author: Alex Richardson <arichardson@FreeBSD.org>
Date:   Tue Jan 19 11:35:05 2021 +0000

    usr.bin/sort: Avoid UBSan errors
    
    UBSan complains about out-of-bounds accesses for zero-length arrays. To
    avoid this we can use flexible array members. However, the C standard does
    not allow for structures that only contain flexible array members, so we
    move the length parameters into that structure too.
`
This revision now requires review to proceed.Jan 22 2021, 2:54 PM
usr.bin/sort/bwstring.h
112

Should fix accesses the non-active union member

jrtc27 retitled this revision from Fix a few UBSan errors in boostrap-tools to Fix a few UBSan errors in bootstrap-tools.Jan 22 2021, 3:02 PM
cddl/contrib/opensolaris/tools/ctf/cvt/ctf.c
1208

What's the reasoning behind this one? You should have size_t * int which will give either int or size_t depending on 32-bit vs 64-bit, I believe, both of which should then work fine as an offset. But if there's something weird going on with that would it not be best to make vlen a size_t instead?

cddl/contrib/opensolaris/tools/ctf/cvt/hash.c
82

This changes based on char signedness?

contrib/elftoolchain/libdwarf/libdwarf_rw.c
287

Is setting the MSB with | less UB than shifting into it?

293

Does this want to be (int64_t)-1 or something instead? Using UINT64_MAX is a bit weird.

cddl/contrib/opensolaris/tools/ctf/cvt/ctf.c
1208

I can't remember what the error was, I think it was that vlen can be zero and UBSan complains that adding a large unsigned number to dptr overflows it.

contrib/elftoolchain/libdwarf/libdwarf_rw.c
293

(int64_t)-1 still shifts the sign bits. We have to use an unsigned type to silence the warning.

cddl/contrib/opensolaris/tools/ctf/cvt/ctf.c
1208

Then the if below should be:

if (vlen > 0 && ((u_short *)dptr)[vlen - 1] == 0)

and this line can go?

contrib/elftoolchain/libdwarf/libdwarf_rw.c
293

Ah, right, the UB is specifically anything that's not left-shifting a non-negative value to give a still-representable result.

contrib/elftoolchain/libdwarf/libdwarf_rw.c
287

I didn't get an error, but it's possible that it would be one. Maybe it just wasn't triggered with the given inputs.