Page MenuHomeFreeBSD
Differential D31009

usr.bin/sort: Avoid UBSan errors
ClosedPublic
Actions

Authored by arichardson on Jul 3 2021, 3:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 21, 3:02 PM
Unknown Object (File)
Sun, Mar 16, 9:28 AM
Unknown Object (File)
Sat, Mar 8, 1:51 AM
Unknown Object (File)
Sat, Mar 8, 1:50 AM
Unknown Object (File)
Sat, Mar 8, 1:37 AM
Unknown Object (File)
Fri, Mar 7, 10:57 PM
Unknown Object (File)
Feb 23 2025, 12:07 PM
Unknown Object (File)
Feb 20 2025, 5:14 AM
View All Files
Subscribers
None

Details

Reviewers
markj
jrtc27
Commits
rGe4c2ffe93205: usr.bin/sort: Avoid UBSan errors
rGd053fb22f6d3: usr.bin/sort: Avoid UBSan errors
Summary

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.

Split out from D28233.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 40251
Build 37140: arc lint + arc unit

Event Timeline

arichardson requested review of this revision.Jul 3 2021, 3:10 PM
arichardson created this revision.
Harbormaster completed remote builds in B40251: Diff 91699.Jul 3 2021, 3:10 PM
arichardson added a parent revision: D31043: Add build system support for ASAN+UBSAN instrumentation.Jul 5 2021, 12:17 PM
markj accepted this revision.Jul 5 2021, 12:24 PM
markj added inline comments.
usr.bin/sort/bwstring.c
170

Why is it + 2?

usr.bin/sort/bwstring.h
78

Can't this be mb_cur_max instead? I guess it ought to be irrelevant since both branches do the same thing.

This revision is now accepted and ready to land.Jul 5 2021, 12:24 PM
arichardson added inline comments.Jul 5 2021, 12:26 PM
usr.bin/sort/bwstring.c
170

Why is it + 2?

I am not sure about this, I would assume + 1 is sufficient for the zero char, but I didn't want to change the existing code in case something depends on the + 2.

usr.bin/sort/bwstring.h
78

Ah good catch, I missed this one while rebasing.

markj added inline comments.Jul 5 2021, 12:38 PM
usr.bin/sort/bwstring.c
170

Yeah, better not to change it. I just stared at it for a while and couldn't see why + 1 wasn't enough.

Closed by commit rGd053fb22f6d3: usr.bin/sort: Avoid UBSan errors (authored by arichardson). · Explain WhyJul 6 2021, 10:07 AM
This revision was automatically updated to reflect the committed changes.
arichardson added a commit: rGd053fb22f6d3: usr.bin/sort: Avoid UBSan errors.
arichardson added a commit: rGe4c2ffe93205: usr.bin/sort: Avoid UBSan errors.Aug 5 2021, 9:26 AM

Revision Contents
Changeset List

PathSize
usr.bin/
sort/
2 lines
31 lines
267 lines

Diff 91699

View Options

usr.bin/sort/Makefile

Show First 20 Lines • Show All 2,966 Lines • ▼ Show 20 Lines
*/ */
bool bool
vm_page_reclaim_contig_domain(int domain, int req, u_long npages, vm_page_reclaim_contig_domain(int domain, int req, u_long npages,
vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary) vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary)
{ {
struct vm_domain *vmd; struct vm_domain *vmd;
vm_paddr_t curr_low; vm_paddr_t curr_low;
vm_page_t m_run, m_runs[NRUNS]; vm_page_t m_run, m_runs[NRUNS];
u_long count, reclaimed; u_long count, minalign, reclaimed;
int error, i, options, req_class; int error, i, options, req_class;
KASSERT(npages > 0, ("npages is 0")); KASSERT(npages > 0, ("npages is 0"));
KASSERT(powerof2(alignment), ("alignment is not a power of 2")); KASSERT(powerof2(alignment), ("alignment is not a power of 2"));
KASSERT(powerof2(boundary), ("boundary is not a power of 2")); KASSERT(powerof2(boundary), ("boundary is not a power of 2"));
alcUnsubmitted
Done
Inline Actions

I think that the rounding should be performed here, right after the KASSERT()s and before the sanity check on the number of free pages. The reason being that in the worst case vm_page_reclaim_run() will perform the rounded up number of page allocations before it frees any of the pages from the contiguous run.

alc: I think that the rounding should be performed here, right after the KASSERT()s and before the…
req_class = req & VM_ALLOC_CLASS_MASK; req_class = req & VM_ALLOC_CLASS_MASK;
/* /*
* The page daemon is allowed to dig deeper into the free page list. * The page daemon is allowed to dig deeper into the free page list.
*/ */
if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT) if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT)
req_class = VM_ALLOC_SYSTEM; req_class = VM_ALLOC_SYSTEM;
/* /*
* Return if the number of free pages cannot satisfy the requested * Return if the number of free pages cannot satisfy the requested
* allocation. * allocation.
*/ */
vmd = VM_DOMAIN(domain); vmd = VM_DOMAIN(domain);
count = vmd->vmd_free_count; count = vmd->vmd_free_count;
if (count < npages + vmd->vmd_free_reserved || (count < npages + if (count < npages + vmd->vmd_free_reserved || (count < npages +
vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) || vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) ||
(count < npages && req_class == VM_ALLOC_INTERRUPT)) (count < npages && req_class == VM_ALLOC_INTERRUPT))
return (false); return (false);
/*
* The caller will attempt an allocation after some runs have been
* reclaimed and added to the vm_phys buddy lists. To ensure that runs
* are not fragmented, round up the requested length to the next power
alcUnsubmitted
Done
Inline Actions

Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of vm_phys_alloc_contig(), ..."

alc: Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of…
* or two or maximum chunk size, and ensure that each run is suitably
* aligned.
*/
minalign = 1ul << imin(flsl(npages - 1), VM_NFREEORDER - 1);
npages = roundup2(npages, minalign);
if (alignment < ptoa(minalign))
alignment = ptoa(minalign);
/* /*
* Scan up to three times, relaxing the restrictions ("options") on * Scan up to three times, relaxing the restrictions ("options") on
* the reclamation of reservations and superpages each time. * the reclamation of reservations and superpages each time.
*/ */
for (options = VPSC_NORESERV;;) { for (options = VPSC_NORESERV;;) {
/* /*
* Find the highest runs that satisfy the given constraints * Find the highest runs that satisfy the given constraints
▲ Show 20 Lines • Show All 2,522 Lines • Show Last 20 Lines
View Options

usr.bin/sort/bwstring.h

Show First 20 Lines • Show All 2,966 Lines • ▼ Show 20 Lines
*/ */
bool bool
vm_page_reclaim_contig_domain(int domain, int req, u_long npages, vm_page_reclaim_contig_domain(int domain, int req, u_long npages,
vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary) vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary)
{ {
struct vm_domain *vmd; struct vm_domain *vmd;
vm_paddr_t curr_low; vm_paddr_t curr_low;
vm_page_t m_run, m_runs[NRUNS]; vm_page_t m_run, m_runs[NRUNS];
u_long count, reclaimed; u_long count, minalign, reclaimed;
int error, i, options, req_class; int error, i, options, req_class;
KASSERT(npages > 0, ("npages is 0")); KASSERT(npages > 0, ("npages is 0"));
KASSERT(powerof2(alignment), ("alignment is not a power of 2")); KASSERT(powerof2(alignment), ("alignment is not a power of 2"));
KASSERT(powerof2(boundary), ("boundary is not a power of 2")); KASSERT(powerof2(boundary), ("boundary is not a power of 2"));
alcUnsubmitted
Done
Inline Actions

I think that the rounding should be performed here, right after the KASSERT()s and before the sanity check on the number of free pages. The reason being that in the worst case vm_page_reclaim_run() will perform the rounded up number of page allocations before it frees any of the pages from the contiguous run.

alc: I think that the rounding should be performed here, right after the KASSERT()s and before the…
req_class = req & VM_ALLOC_CLASS_MASK; req_class = req & VM_ALLOC_CLASS_MASK;
/* /*
* The page daemon is allowed to dig deeper into the free page list. * The page daemon is allowed to dig deeper into the free page list.
*/ */
if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT) if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT)
req_class = VM_ALLOC_SYSTEM; req_class = VM_ALLOC_SYSTEM;
/* /*
* Return if the number of free pages cannot satisfy the requested * Return if the number of free pages cannot satisfy the requested
* allocation. * allocation.
*/ */
vmd = VM_DOMAIN(domain); vmd = VM_DOMAIN(domain);
count = vmd->vmd_free_count; count = vmd->vmd_free_count;
if (count < npages + vmd->vmd_free_reserved || (count < npages + if (count < npages + vmd->vmd_free_reserved || (count < npages +
vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) || vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) ||
(count < npages && req_class == VM_ALLOC_INTERRUPT)) (count < npages && req_class == VM_ALLOC_INTERRUPT))
return (false); return (false);
/*
* The caller will attempt an allocation after some runs have been
* reclaimed and added to the vm_phys buddy lists. To ensure that runs
* are not fragmented, round up the requested length to the next power
alcUnsubmitted
Done
Inline Actions

Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of vm_phys_alloc_contig(), ..."

alc: Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of…
* or two or maximum chunk size, and ensure that each run is suitably
* aligned.
*/
minalign = 1ul << imin(flsl(npages - 1), VM_NFREEORDER - 1);
npages = roundup2(npages, minalign);
if (alignment < ptoa(minalign))
alignment = ptoa(minalign);
/* /*
* Scan up to three times, relaxing the restrictions ("options") on * Scan up to three times, relaxing the restrictions ("options") on
* the reclamation of reservations and superpages each time. * the reclamation of reservations and superpages each time.
*/ */
for (options = VPSC_NORESERV;;) { for (options = VPSC_NORESERV;;) {
/* /*
* Find the highest runs that satisfy the given constraints * Find the highest runs that satisfy the given constraints
▲ Show 20 Lines • Show All 2,522 Lines • Show Last 20 Lines
View Options

usr.bin/sort/bwstring.c

Show First 20 Lines • Show All 2,966 Lines • ▼ Show 20 Lines
*/ */
bool bool
vm_page_reclaim_contig_domain(int domain, int req, u_long npages, vm_page_reclaim_contig_domain(int domain, int req, u_long npages,
vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary) vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary)
{ {
struct vm_domain *vmd; struct vm_domain *vmd;
vm_paddr_t curr_low; vm_paddr_t curr_low;
vm_page_t m_run, m_runs[NRUNS]; vm_page_t m_run, m_runs[NRUNS];
u_long count, reclaimed; u_long count, minalign, reclaimed;
int error, i, options, req_class; int error, i, options, req_class;
KASSERT(npages > 0, ("npages is 0")); KASSERT(npages > 0, ("npages is 0"));
KASSERT(powerof2(alignment), ("alignment is not a power of 2")); KASSERT(powerof2(alignment), ("alignment is not a power of 2"));
KASSERT(powerof2(boundary), ("boundary is not a power of 2")); KASSERT(powerof2(boundary), ("boundary is not a power of 2"));
alcUnsubmitted
Done
Inline Actions

I think that the rounding should be performed here, right after the KASSERT()s and before the sanity check on the number of free pages. The reason being that in the worst case vm_page_reclaim_run() will perform the rounded up number of page allocations before it frees any of the pages from the contiguous run.

alc: I think that the rounding should be performed here, right after the KASSERT()s and before the…
req_class = req & VM_ALLOC_CLASS_MASK; req_class = req & VM_ALLOC_CLASS_MASK;
/* /*
* The page daemon is allowed to dig deeper into the free page list. * The page daemon is allowed to dig deeper into the free page list.
*/ */
if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT) if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT)
req_class = VM_ALLOC_SYSTEM; req_class = VM_ALLOC_SYSTEM;
/* /*
* Return if the number of free pages cannot satisfy the requested * Return if the number of free pages cannot satisfy the requested
* allocation. * allocation.
*/ */
vmd = VM_DOMAIN(domain); vmd = VM_DOMAIN(domain);
count = vmd->vmd_free_count; count = vmd->vmd_free_count;
if (count < npages + vmd->vmd_free_reserved || (count < npages + if (count < npages + vmd->vmd_free_reserved || (count < npages +
vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) || vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) ||
(count < npages && req_class == VM_ALLOC_INTERRUPT)) (count < npages && req_class == VM_ALLOC_INTERRUPT))
return (false); return (false);
/*
* The caller will attempt an allocation after some runs have been
* reclaimed and added to the vm_phys buddy lists. To ensure that runs
* are not fragmented, round up the requested length to the next power
alcUnsubmitted
Done
Inline Actions

Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of vm_phys_alloc_contig(), ..."

alc: Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of…
* or two or maximum chunk size, and ensure that each run is suitably
* aligned.
*/
minalign = 1ul << imin(flsl(npages - 1), VM_NFREEORDER - 1);
npages = roundup2(npages, minalign);
if (alignment < ptoa(minalign))
alignment = ptoa(minalign);
/* /*
* Scan up to three times, relaxing the restrictions ("options") on * Scan up to three times, relaxing the restrictions ("options") on
* the reclamation of reservations and superpages each time. * the reclamation of reservations and superpages each time.
*/ */
for (options = VPSC_NORESERV;;) { for (options = VPSC_NORESERV;;) {
/* /*
* Find the highest runs that satisfy the given constraints * Find the highest runs that satisfy the given constraints
▲ Show 20 Lines • Show All 2,522 Lines • Show Last 20 Lines