Thanks to Patrick Steinhardt for reviewing v2.
Changes since v2:
- Patch 1: drop the redundant file size check since the OID printed by
`hash-object -w` already proves the content is complete, using
`cat-file -e` just to verify the object exists. This also avoids
reading and writing the same file unnecessarily.
- Patch 2 & 3: unchanged.
- Patch 4: shortened commit message following Patrick's comments.
Link to v2:
https://lore.kernel.org/git/cover.1793352943.git.gitter.spiros@gmail.com/
Elia Pinto (4):
odb: use size_t for the length of objects to write
diff: use size_t for the allocation size of word-diff buffers
convert: use size_t for buffer sizes
convert: use size_t for text statistics and ident counts
attr.c | 2 +-
convert.c | 24 ++++++++++++------------
diff.c | 4 ++--
odb.c | 2 +-
odb.h | 4 ++--
odb/source-inmemory.c | 2 +-
odb/source-loose.c | 2 +-
odb/source.h | 2 +-
read-cache-ll.h | 2 +-
read-cache.c | 4 ++--
t/t1007-hash-object.sh | 11 +++++++++++
11 files changed, 35 insertions(+), 24 deletions(-)
Range-diff against v2:
1: 8fcb9f10f1 ! 1: 4961a1824a odb: use size_t for the length of objects to write
@@ Commit message
The existing "hash filtered files over 4GB correctly" test in t1007
only hashes, so add a variant that writes the object with "git
- hash-object -w" and checks both its object ID and its size. Like the
+ hash-object -w" and checks its object ID and that it exists. Like the
other tests over 4GB, it needs EXPENSIVE and SIZE_T_IS_64BIT. Truncating
the length to 32 bits in odb_write_object_ext(), as happens on Windows
without this patch, makes it fail with a different object ID.
@@ t/t1007-hash-object.sh: test_expect_success EXPENSIVE,SIZE_T_IS_64BIT \
+ echo "big filter=null-filter" >.gitattributes &&
+ git hash-object -w -- big >actual &&
+ test_cmp expect actual &&
-+ echo $((5*1024*1024*1024)) >expect &&
-+ git cat-file -s $(cat actual) >actual &&
-+ test_cmp expect actual
++ git cat-file -e "$(cat actual)"
+'
+
test_done
2: 42d4eae985 = 2: b27ef2a888 diff: use size_t for the allocation size of word-diff buffers
3: 88ca2ceb80 = 3: 1617177880 convert: use size_t for buffer sizes
4: 84e9e38a3e ! 4: 153463b531 convert: use size_t for text statistics and ident counts
@@ Metadata
## Commit message ##
convert: use size_t for text statistics and ident counts
- gather_stats() counts NULs, CRs, LFs, CRLFs and (non)printable
- characters in "unsigned" fields of struct text_stat, and
- count_ident() counts "$Id$" markers in an "int". Now that the buffer
- sizes they work on are size_t, a large enough buffer overflows them.
- Unlike the previous patches, this is not specific to LLP64: "unsigned"
- is 32 bits wide on all the platforms we care about.
+ gather_stats() and count_ident() use "unsigned" and "int" counters to
+ count characters and markers in buffers whose sizes are measured in
+ size_t. On platforms where size_t can exceed 32 bits, large inputs can
+ overflow these counters.
- The counters only drive decisions, but a wrapped counter makes the
- wrong one. For example, a file with exactly 2^32 lone LFs gets
- lonelf == 0, so will_convert_lf_to_crlf() decides there is nothing
- to convert, and a wrapped "printable" or "nonprintable" can fool the
- heuristic in convert_is_binary(). In count_ident(), going past
- INT_MAX is undefined behaviour.
+ For example, a file with exactly 2^32 lone LFs wraps lonelf back
+ to zero, causing will_convert_lf_to_crlf() to miss the conversion.
- This is not a memory safety problem: the counts are only used as
- hints to strbuf_grow() in crlf_to_worktree() and ident_to_worktree(),
- and the buffers are then filled with strbuf_add(), which grows them
- as needed.
+ Switch the counters to size_t and check the size hint for overflow.
- Use size_t for all of them, and compute the size hint in
- ident_to_worktree() with st_add() and st_mult(), so that it dies
- instead of wrapping around.
-
- Triggering any of this needs more than 4GiB of input, so no test is
- added.
+ Triggering this requires processing over 4GiB of input, so no test
+ is added.
Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>base-commit: b9e5cff1803ab24582201ef059516eb16014b9d9
Elia Pinto <gitter.spiros@gmail.com>