| Message ID | cover.1544729841.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add commit-graph fuzzer and fix buffer overflow | expand |
On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote: > Add a new fuzz test for the commit graph and fix a buffer read-overflow > that it discovered. Additionally, fix the Makefile instructions for > building fuzzers. > > Changes since V3: > * Improve portability of the new test functionality. I thought there was some question about /dev/zero, which I think is in this version (I don't actually know whether there are portability issues or not, but somebody did mention it). -Peff
On 2018.12.18 12:35, Jeff King wrote: > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote: > > > Add a new fuzz test for the commit graph and fix a buffer read-overflow > > that it discovered. Additionally, fix the Makefile instructions for > > building fuzzers. > > > > Changes since V3: > > * Improve portability of the new test functionality. > > I thought there was some question about /dev/zero, which I think is > in this version (I don't actually know whether there are portability > issues or not, but somebody did mention it). > > -Peff I've only found one reference [1] (from 1999) of OS X Server not having a /dev/zero. It appears to be present as of 2010 though [2]. [1]: https://macosx-admin.omnigroup.narkive.com/sAxj0XeP/dev-zero-equivalent-in-mac-os-x [2]: https://serverfault.com/questions/143248/zeroing-a-disk-with-dd-vs-disk-utility
On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote: > On 2018.12.18 12:35, Jeff King wrote: > > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote: > > > > > Add a new fuzz test for the commit graph and fix a buffer read-overflow > > > that it discovered. Additionally, fix the Makefile instructions for > > > building fuzzers. > > > > > > Changes since V3: > > > * Improve portability of the new test functionality. > > > > I thought there was some question about /dev/zero, which I think is > > in this version (I don't actually know whether there are portability > > issues or not, but somebody did mention it). > > > > -Peff > > I've only found one reference [1] (from 1999) of OS X Server not having > a /dev/zero. It appears to be present as of 2010 though [2]. Thanks for digging. That seems like enough to assume we should try it and see if any macOS people complain. I do wonder if we'll run into problems on Windows, though. -Peff
Hi Peff, On Wed, 19 Dec 2018, Jeff King wrote: > On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote: > > > On 2018.12.18 12:35, Jeff King wrote: > > > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote: > > > > > > > Add a new fuzz test for the commit graph and fix a buffer read-overflow > > > > that it discovered. Additionally, fix the Makefile instructions for > > > > building fuzzers. > > > > > > > > Changes since V3: > > > > * Improve portability of the new test functionality. > > > > > > I thought there was some question about /dev/zero, which I think is > > > in this version (I don't actually know whether there are portability > > > issues or not, but somebody did mention it). > > > > > > -Peff > > > > I've only found one reference [1] (from 1999) of OS X Server not having > > a /dev/zero. It appears to be present as of 2010 though [2]. > > Thanks for digging. That seems like enough to assume we should try it > and see if any macOS people complain. > > I do wonder if we'll run into problems on Windows, though. As long as we're talking about Unix shell scripts, /dev/zero should be fine, as we are essentially running in a variant of Cygwin. If you try to pass /dev/zero as an argument to a Git command, that's an entirely different thing: this most likely won't work. Ciao, Dscho
On Thu, Dec 20, 2018 at 08:35:57PM +0100, Johannes Schindelin wrote: > > I do wonder if we'll run into problems on Windows, though. > > As long as we're talking about Unix shell scripts, /dev/zero should be > fine, as we are essentially running in a variant of Cygwin. > > If you try to pass /dev/zero as an argument to a Git command, that's an > entirely different thing: this most likely won't work. Thanks for confirming. We're talking about passing it to dd here, so I think it should be OK. -Peff
Jeff King <peff@peff.net> writes: > On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote: > >> On 2018.12.18 12:35, Jeff King wrote: >> > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote: >> > >> > > Add a new fuzz test for the commit graph and fix a buffer read-overflow >> > > that it discovered. Additionally, fix the Makefile instructions for >> > > building fuzzers. >> > > >> > > Changes since V3: >> > > * Improve portability of the new test functionality. >> > >> > I thought there was some question about /dev/zero, which I think is >> > in this version (I don't actually know whether there are portability >> > issues or not, but somebody did mention it). >> > >> > -Peff >> >> I've only found one reference [1] (from 1999) of OS X Server not having >> a /dev/zero. It appears to be present as of 2010 though [2]. > > Thanks for digging. That seems like enough to assume we should try it > and see if any macOS people complain. Our tests have been relying on /dev/zero since 852a1710 ("am: let command-line options override saved options", 2015-08-04) that appeared in v2.6.0. Anybody who has trouble with /dev/zero now has kept silent for about a dozen major releases, I think, and will be silent with this one, too ;-) > > I do wonder if we'll run into problems on Windows, though. > > -Peff
Add a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the Makefile instructions for building fuzzers. Changes since V3: * Improve portability of the new test functionality. * Fix broken &&-chains in tests. Changes since V2: * Avoid pointer arithmetic overflow when checking the graph's chunk count. * Merge the corrupt_graph_and_verify and corrupt_and_zero_graph_then_verify test functions. Josh Steadmon (3): commit-graph, fuzz: Add fuzzer for commit-graph commit-graph: fix buffer read-overflow Makefile: correct example fuzz build .gitignore | 1 + Makefile | 3 +- commit-graph.c | 67 +++++++++++++++++++++++++++++------------ commit-graph.h | 3 ++ fuzz-commit-graph.c | 16 ++++++++++ t/t5318-commit-graph.sh | 16 ++++++++-- 6 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 fuzz-commit-graph.c Range-diff against v3: 1: 675d58ecea ! 1: 80b5662f30 commit-graph: fix buffer read-overflow @@ -55,29 +55,26 @@ pos=$1 data="${2:-\0}" grepstr=$3 -+ orig_size=$(stat --format=%s $objdir/info/commit-graph) -+ zero_pos=${4:-${orig_size}} ++ orig_size=$(wc -c < $objdir/info/commit-graph) && ++ zero_pos=${4:-${orig_size}} && cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && -+ truncate --size=$zero_pos $objdir/info/commit-graph && -+ truncate --size=$orig_size $objdir/info/commit-graph && ++ dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && ++ dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) && test_must_fail git commit-graph verify 2>test_err && - grep -v "^+" test_err >err +- grep -v "^+" test_err >err ++ grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err } -+ - test_expect_success 'detect bad signature' ' - corrupt_graph_and_verify 0 "\0" \ - "graph signature" @@ "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' -+ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ ++ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + 2: 06a32bfe8b = 2: 21101b961a Makefile: correct example fuzz build