Page MenuHomeFreeBSD

xargs: Avoid a possible multiplication overflow
AcceptedPublic

Authored by jlduran on Jul 14 2023, 10:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 13 2024, 10:48 PM
Unknown Object (File)
Sep 24 2024, 3:16 AM
Unknown Object (File)
Sep 19 2024, 5:24 PM
Unknown Object (File)
Sep 19 2024, 11:19 AM
Unknown Object (File)
Sep 18 2024, 9:48 PM
Unknown Object (File)
Sep 18 2024, 8:55 AM
Unknown Object (File)
Sep 8 2024, 7:41 PM
Unknown Object (File)
Sep 1 2024, 9:47 PM

Details

Reviewers
des
emaste
Group Reviewers
Klara
Summary

Use calloc() instead of malloc() to avoid a possible multiplication overflow.


NB: This was the cheapest way I was able to make the test introduced in eab91d008165e7bbf8ca7b87eabe4dc8bf3da191 pass.

Test Plan
# kyua debug legacy_test:main
1..22
ok - normal # Test detected no regression. (in .)
ok - I # Test detected no regression. (in .)
ok - J # Test detected no regression. (in .)
ok - L # Test detected no regression. (in .)
ok - P1 # Test detected no regression. (in .)
ok - R # Test detected no regression. (in .)
ok - R-1 # Test detected no regression. (in .)
ok - n1 # Test detected no regression. (in .)
ok - n2 # Test detected no regression. (in .)
ok - n2147483647 # Test detected no regression. (in .)
ok - n2P0 # Test detected no regression. (in .)
ok - n3 # Test detected no regression. (in .)
ok - 0 # Test detected no regression. (in .)
ok - 0I # Test detected no regression. (in .)
ok - 0J # Test detected no regression. (in .)
ok - 0L # Test detected no regression. (in .)
ok - 0P1 # Test detected no regression. (in .)
ok - quotes # Test detected no regression. (in .)
ok - parallel1 # Test detected no regression. (in .)
ok - parallel2 # Test detected no regression. (in .)
ok - parallel3 # Test detected no regression. (in .)
ok - parallel4 # Test detected no regression. (in .)
legacy_test:main  ->  passed

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

This revision is now accepted and ready to land.Jul 17 2023, 4:03 PM
markj added inline comments.
usr.bin/xargs/xargs.c
261

The problem is here, and it's triggered by xargs -n 2147483647. That causes xargs to allocate 16GB of RAM.

The problem is that with malloc(), by default, jemalloc will fill the allocated memory with a pattern that makes it a bit easier to detect use-after-frees. calloc() returns zero-filled memory, so it can just call mmap() and return the result without triggering any page faults.

Does it make sense to allow such large values for -n? Wouldn't arg_max also work?

usr.bin/xargs/xargs.c
261

Thank you for clarifying.
The only reason I presented this revision was to make te test pass. I also have a restricted testing environment, and the test you just disabled was crashing it.

One other possible solution would be something like this (untested):

-# This test may consume a large amount of memory, making it unsuited to CI
-# environments.  Disable it for now.
-#REGRESSION_TEST(`n2147483647', `xargs -n2147483647 <${SRCDIR}/regress.in')
+REGRESSION_TEST(`n2147483647', `MALLOC_CONF=junk:false xargs -n2147483647 <${SRCDIR}/regress.in')

That assumes that jemalloc is the malloc implementation in use, but I suspect it's harmless otherwise.