Teach Git to handle huge files in smudge/clean

[vtbassmatt]

On platforms like Windows, where unsigned long is 32 bits, Git can only deal with files up to 4GB. This is fine for normal objects, since no one should commit such large files to Git. However, even external solutions like Git LFS are subject to the limitation, since smudge/clean also make use of unsigned long.

This patch series introduces a test case exposing the problem, then makes the minimum changes necessary for the test to pass on such platforms.

[vtbassmatt]

I wrote this in a way to localize the changes to a small area. Personally I find the solution a little ugly, and wouldn't mind extending the changes a bit farther out. But you don't get very far down that path before you've got to turn object_info.sizep into a size_t, which has extensive implications.

Thoughts / comments welcomed. If I get a chance this weekend, I'll try introducing a commit which makes those larger changes, simply for discussion.

(Oh -- and I couldn't verify this truly fixes the issue on Windows 😢 My VM is too small and runs out of both memory and swap space, so it dies to that. But I validated that the changes at least compile without warning.)

[PhilipOakley]

Hi Matt,
It's good to see some progress in this tricky area.

I'm not a user of git-lfs so I'm a bit confused by some of the test approach.

Could the commit message include a little bit more background as to how the smudge/clean filters are used in LFS, and the separation of concerns as to where the 'failure' that is being tested resides. (e.g. areas of: Git, LFS, the filters, bash, .. [or maybe some call back from LFS into git?])

To me (in ignorance), we have a very small file that is being stored in the git repo which has no ulong=32bit limitations. We then have external filters that massage the file (external 'bash' commands?) from being a short file to being a very large file (breaking the ulong=32bit barrier). And a filter to reverse that. Somehow, those filters are corrupting the end to end loop-back testing, but it's unclear to me (i.e. there is some info I'm missing) where git itself is involved in those steps.

Reminding myself: The “clean” filter is run when files are staged. The “smudge” filter is run on checkout (https://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes fig 143/144 ;-)

Ideally, the commit message (to inform future readers/helpers & me) should also indicate where, within Git, you/we think that the failing is located. At the moment, I couldn't tell.

Philip

[PhilipOakley]

Could the commit message include a little bit more background as to how the smudge/clean filters are used in LFS, and the separation of concerns as to where the 'failure' that is being tested resides. (e.g. areas of: Git, LFS, the filters, bash, .. [or maybe some call back from LFS into git?])

A bit more searching, and serendipity, got me to git smudge clean interface suboptiomal which does look to include some cross-related info about git-lfs uses the interface and how both git-annex and git-lfs can have a struggle with large files.

Perhaps some of their core insights could be extracted?

[vtbassmatt]

@PhilipOakley thank you for the feedback. (Genuine "thank you", not the "thanks" you described at the Contributors Summit 😋) As this is my first contribution, it's hard to know exactly what's worth stating and what's well-known to reviewers. I'll flesh out my descriptions to make this more self-explanatory.

[vtbassmatt]

Hopefully these longer commit messages better motivate the change. I also realized that the belt-and-suspenders guard (checking for very large objects being checked out from the repository) may not be strictly necessary, so I split it into its own final commit.

[PhilipOakley]

Aside: the links in the GitHub notification email appear to be in error as they link to the git repo, rather than your fork. e.g. This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository. I guess this is a GitHub problem, but the links in the emails will stay wrong ;-)

[PhilipOakley]

I guess this is a GitHub problem

Maybe it was the force push.. (still OK)

[PhilipOakley]

(sticking my neck out here):
I think that trunc_size in the if should be up cast back to size_t so that we are explicit about what we intend, rather than assume the 'side-effects' of coercion between independent types. Others may disagree..

[dscho]

No, we're not relying on a side effect here by using unsigned long. Instead, we are testing precisely what the problem is. The only problem I could see is that some static analysis running on a 64-bit Linux would complain that this check is "always false". We would have to work around such a warning, but we do not need to upcast the trunc_size again (that would even risk overzealous optimization in buggy C implementations).

[PhilipOakley]

We are relying on the fact that two different 'implementation defined' types will convert the way we hope. The implementation defined 'gotchas' are not quite as bad as undefined behaviours, but can catch us all out (which is why we are here).

Maybe this has to be thrown at the "naming is hard" side of what the temp variable should be called so as to "obviously", at a glance, inform the reader of it's purpose, maybe windows_long_size, or even shortened_long_size ?

Or maybe just a short comment about the LLP64 issue it's catching (we do want it to go away in the long term).

[dscho]

The problem is simple: when size_t is wider than unsigned long, the cast to the latter might cut off bits. The end result is that the cast value is different from the original value. And that is precisely what the code in question tested.

So I don't see any way to improve the clarity of this code.

Casting the value back to size_t explicitly before comparing only would make things confusing again.

[PhilipOakley]

Can the commit message be split into separate paragraphs rather than a wall of text?

Be explicit about having checked (or not) all the callers for the changed function.

Discussion point: can we create a fresh typedef (e.g. memlen, still to ulong, to indicate the places where future changes will be needed (e.g. those function casts). Let's use the name equivalence of typedefs to our benefit.

[PhilipOakley]

Can the commit message be split into paragraphs rather than a wall of text? Including just after "buffer the entire smudged file into memory." which is (IIUC) the Git internals area.

[dscho]

@vtbassmatt congratulations:

[...]
++ test_cleanup='{ test_unconfig  '\''filter.makelarge.smudge'\''
                } && (exit "$eval_ret"); eval_ret=$?; { test_unconfig  '\''filter.makelarge.clean'\''
                } && (exit "$eval_ret"); eval_ret=$?; :'
++ git config filter.makelarge.smudge 'dd if=/dev/zero bs=1048576 count=5120 && cat'
++ echo 'small filter=makelarge'
++ rm small
++ git checkout -- small
5120+0 records in
5120+0 records out
5368709120 bytes (5.4 GB, 5.0 GiB) copied, 16.7433 s, 321 MB/s
+++ test_file_size small
+++ test 1 -ne 1
+++ test-tool path-utils file-size small
++ size=5368709133
++ test 5368709133 -ge 5368709120
++ test_unconfig filter.makelarge.smudge
++ config_dir=
++ test filter.makelarge.smudge = -C
++ git config --unset-all filter.makelarge.smudge
++ config_status=0
++ case "$config_status" in
++ return 0
++ exit 0
++ eval_ret=0
++ test_unconfig filter.makelarge.clean
++ config_dir=
++ test filter.makelarge.clean = -C
++ git config --unset-all filter.makelarge.clean
++ config_status=0
++ case "$config_status" in
++ return 0
++ exit 0
++ eval_ret=0
++ :
ok 11 - files over 4GB convert on output

# passed all 11 test(s)
1..11

It works!

Now, I would like to caution that Git LFS uses the process attribute, i.e. it uses a slightly different approach than the regular smudge/clean filters. I haven't analyzed things in detail yet, but it would seem as if Git's test suite uses only a single process filter for testing. It is in t/t0021/rot13-filter.pl, which is a 247-line Perl script that might not exactly easy to read for the faint of heart. The biggest complication of implementing such a filter just for testing is that it needs to "speak" Git's "pkt-line" protocol. Quite honestly, I would have preferred the test filter to be implemented in the form of a test tool, i.e. added to t/helper/ and implemented in pure C, using libgit.a. But given the bias toward Perl of the most prolific Git contributors, I am not surprised.

So maybe in the short run I could ask you to set up a manual test repository on GitHub that has a 4.5GB blob served via Git LFS? That way, we should be able to verify this fix "on the cheap".

I've got a fixup to address https://github.com/git-for-windows/git/runs/3989339996?check_suite_focus=true#step:4:566, but am currently investigating https://github.com/git-for-windows/git/runs/3989354647?check_suite_focus=true#step:6:521 (and therefore will hold off from pushing my fixup just yet, in case I figure out another one to push on top).

[vtbassmatt]

@PhilipOakley yes, my apologies on the wall of text. I struggled to capture what I wanted to say, but might be able to articulate it better this morning.

@dscho thank you very much! I'll get a test repo set up so we can do a quick validation. I also woke up this morning realizing that I've proven and fixed the smudge side, but not so much the clean side. I think the test looks like the inverse of the current one: create a file with 5GB of leading \0 followed by a bit of content; commit it with the makelarge.clean filter, then disable filters and force a checkout. Should that test be in the same commit, in a new commit, or in a whole new series? (And how do I make this decision on my own in the future?)

[dscho]

Okay, I was investigating the vs-test (and with my fixup, also windows-test) failures. My immediate gut feeling was that this might have something to do with size being assigned -1 (which would obviously translate to all bits set, i.e. the maximum size_t value). But that turned out to be wrong, it is much simpler than that: it is uninitialized memory. Running ./t3400-*.sh --valgrind-only=30 --run=1-31 on Linux reports this:

[...]
+ git rebase main
==17796== Conditional jump or move depends on uninitialised value(s)
==17796==    at 0x2AE121: read_blob_entry (entry.c:91)
==17796==    by 0x2AEAD6: write_entry (entry.c:329)
==17796==    by 0x2AF5DA: checkout_entry_ca (entry.c:563)
==17796==    by 0x3D97A0: checkout_entry (entry.h:42)
==17796==    by 0x3DA8B9: check_updates (unpack-trees.c:472)
==17796==    by 0x3DDF4B: unpack_trees (unpack-trees.c:1849)
==17796==    by 0x41BF73: checkout (merge-ort.c:4042)
==17796==    by 0x41C604: merge_switch_to_result (merge-ort.c:4180)
==17796==    by 0x38C19E: do_recursive_merge (sequencer.c:652)
==17796==    by 0x390D31: do_pick_commit (sequencer.c:2223)
==17796==    by 0x39827A: pick_commits (sequencer.c:4337)
==17796==    by 0x39CF74: complete_action (sequencer.c:5679)
==17796==  Uninitialised value was created by a stack allocation
==17796==    at 0x2AE833: write_entry (entry.c:276)
==17796==

If you look at entry.c:329, you will see that it passes the pointer to size to read_blob_entry(), but nothing in the code path since declaring, but not initializing size_t size; initialized it.

Which makes sense, as we do not know the size of the object before reading it, it is therefore the responsibility of read_object_file() to initialize the value, hence the need to pass a pointer to it.

Now, it is quite the rabbit hole to follow the call chain of read_object_file() right down to the code that assigns the size value:

• read_object_file()
  • repo_read_object_file()
    • read_object_file_extended()
      • read_object()
        • oid_object_info_extended()
          • do_oid_object_info_extended()
            • loose_object_info()
              • parse_loose_header_extended()
            • packed_object_info()
              • cache_or_unpack_entry()
                • unpack_entry()
                  • unpack_object_header()
                  • -> unpack_object_header_buffer()
              • get_size_from_delta()
                • get_delta_hdr_size()

I am in the process of crafting a fixup! to handle the leaves of this call tree.

[vtbassmatt]

Which makes sense, as we do not know the size of the object before reading it, it is therefore the responsibility of read_object_file() to initialize the value, hence the need to pass a pointer to it.

I can't figure out if I broke this or if it was already broken?

Also, I'm rewording the commit messages to be clearer and less "wall of text"y. I'll try to make sure I don't destroy your fixup! in the process.

[dscho]

@PhilipOakley yes, my apologies on the wall of text. I struggled to capture what I wanted to say, but might be able to articulate it better this morning.

I always find it better to reply with a concrete suggestion how the end result should look like. However, in this case I am the only non-native speaker of the three of us, so I will refrain from doing that in this instance.

@dscho thank you very much! I'll get a test repo set up so we can do a quick validation.

Thank you! I saw from the docs that the free GitHub accounts do not allow that, but you probably have the means to get close to the (hard) 5GB limit.

I also woke up this morning realizing that I've proven and fixed the smudge side, but not so much the clean side.

Good point! This could be potentially much harder, if Git insists on reading everything into memory. But I could imagine that it might not be an issue because IIRC Git has some smarts to use a "streaming" interface when git adding large local files.

I think the test looks like the inverse of the current one: create a file with 5GB of leading \0 followed by a b>it of content; commit it with the makelarge.clean filter, then disable filters and force a checkout.

I would actually go for something slightly different. In a new test case, create a checklarge.clean filter that verifies that the size of its input is identical to the file size. Its stdout can be empty, as we are not really interested in the "clean" OID.

Should that test be in the same commit, in a new commit, or in a whole new series? (And how do I make this decision on my own in the future?)

I would say in a new commit.

My general rule: related code changes should be in the same patch series, but each commit should be as fine-grained as possible while still standing on its own. The smudge test case can stand on its own, demonstrating a problem. The fix can stand on its own. And the test case demonstrating the clean case can also stand on its own.

This is a little bit muddy, I know, as it often comes down to a matter of taste. For example, Junio often likes the test cases to be committed together with the corresponding fix (at least if both are small-enough code changes).

[PhilipOakley]

wall of text. I struggled to capture what I wanted to say, but might be able to articulate it better this morning.

I find it easiest to simply add a newline/paragraph after every sentence (fullstop/period), and then take out one or two later when it's obvious that two sentences go together.

and it's easier with the single long sentences to see the over-long sentences that have too many and/or/e.g./i.e. conjunctions that make them unreadable (reading age too high) and hence its easier to see and remove the conjunctions and make separate sentences. (sic).

But simply, new paragraph after every sentence is 90% of the win ;-)

[dscho]

But simply, new paragraph after every sentence is 90% of the win ;-)

I actually do not think so. Paragraphs should be separated semantically. It might often match your rule, but only by happenstance.

[PhilipOakley]

@PhilipOakley yes, my apologies on the wall of text. I struggled to capture what I wanted to say, but might be able to articulate it better this morning.

I always find it better to reply with a concrete suggestion how the end result should look like. However, in this case I am the only non-native speaker of the three of us, so I will refrain from doing that in this instance.

@dscho, I should have realised that my suggestion was mid sentence, which human error theory says is the most likely that readers miss.

I wasn't clear enough about what I meant by "split into separate paragraphs". I added an extended reply above.

[PhilipOakley]

happenstance

It doesn't go the other way though. And once separated, it is much easier for the speaker (native or not) to see where helpful joins occur. Semantically, they are all about the same patch ;-)

[dscho]

happenstance

It doesn't go the other way though. And once separated, it is much easier for the speaker (native or not) to see where helpful joins occur. Semantically, they are all about the same patch ;-)

Honestly, I feel that this thread has devolved into a lot of un-actionable talk at this point. @PhilipOakley I would find it more helpful if you rephrased the commit message and then offered the result as a suggestion.

[dscho]

I am going into full action gear myself, to get this thread back on track. @vtbassmatt what do you think of this?

read_blob_entry: use size_t instead of unsigned long

There is mixed use of `size_t` and `unsigned long` to deal with sizes in the
codebase.

Recall that Windows defines `unsigned long` as 32 bits even on 64-bit
platforms, meaning that converting `size_t` to `unsigned long` narrows the
range. This mostly doesn't cause a problem since Git rarely deals with files
larger than 2^32 bytes.

But adjunct systems such as Git LFS, which use smudge/clean filters to keep
huge files out of the repository, may have huge file contents passed through
some of the functions in `entry.c` and `convert.c`.

On Windows, this results in a truncated file being written to the work tree.
This is due to one specific use of `unsigned long` in `write_entry()` (and a
similar instance in `write_pc_item_to_fd()` for parallel checkout), which
appear to be calling `read_blob_entry()` with a pointer to `unsigned long` for
convenience.

By altering the signature of `read_blob_entry()` to expect a `size_t`,
`write_entry()` can be switched to use `size_t` internally (which all of its
callers and most of its callees already used).

Note: this only solves the problem when a `smudge` produces a file larger than
`unsigned long` can describe. This commit does not touch on the problem where
blob contents larger than `unsigned long` can handle (but `size_t` could) are
handled improperly by Git.

[vtbassmatt]

I like it! I'll fix when I get a chance.

…

On Mon, Oct 25, 2021 at 7:35 AM Johannes Schindelin < ***@***.***> wrote: I am going into full action gear myself, to get this thread back on track. @vtbassmatt <https://github.com/vtbassmatt> what do you think of this? read_blob_entry: use size_t instead of unsigned long There is mixed use of `size_t` and `unsigned long` to deal with sizes in the codebase. Recall that Windows defines `unsigned long` as 32 bits even on 64-bit platforms, meaning that converting `size_t` to `unsigned long` narrows the range. This mostly doesn't cause a problem since Git rarely deals with files larger than 2^32 bytes. But adjunct systems such as Git LFS, which use smudge/clean filters to keep huge files out of the repository, may have huge file contents passed through some of the functions in `entry.c` and `convert.c`. On Windows, this results in a truncated file being written to the work tree. This is due to one specific use of `unsigned long` in `write_entry()` (and a similar instance in `write_pc_item_to_fd()` for parallel checkout), which appear to be calling `read_blob_entry()` with a pointer to `unsigned long` for convenience. By altering the signature of `read_blob_entry()` to expect a `size_t`, `write_entry()` can be switched to use `size_t` internally (which all of its callers and most of its callees already used). Note: this only solves the problem when a `smudge` produces a file larger than `unsigned long` can describe. This commit does not touch on the problem where blob contents larger than `unsigned long` can handle (but `size_t` could) are handled improperly by Git. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3487 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFOMK3GIHFCFVDMK2EXIFDUIU6HRANCNFSM5GQ7GGAA> .

[dscho]

I like it! I'll fix when I get a chance.

I also fixed the compile error, and the test failures. I pushed up the result to your branch, mostly as fixup! commits.

You might like the commit message I crafted for f9b622e.

[dscho]

Discussion point: can we create a fresh typedef (e.g. memlen, still to ulong, to indicate the places where future changes will be needed (e.g. those function casts). Let's use the name equivalence of typedefs to our benefit.

This would be dramatically out of scope for this PR.

[dscho]

Which makes sense, as we do not know the size of the object before reading it, it is therefore the responsibility of read_object_file() to initialize the value, hence the need to pass a pointer to it.

I can't figure out if I broke this or if it was already broken?

@vtbassmatt no, the intention of the original code is to let read_object_file() initialize the value. Hence passing around the pointer to an as-yet-uninitialized value.

[dscho]

@vtbassmatt thank you for sharing access to a Git LFS-enabled test repository with me privately. I can confirm that the 5GB file fails to check out with the current Git for Windows version:

me@work MINGW64 ~/repros/large-file-lfs-3487/lfs-testing (master)
$ git switch large-file
Switched to a new branch 'large-file'
Branch 'large-file' set up to track remote branch 'large-file' from 'origin'.
Encountered 1 file(s) that may not have been copied correctly on Windows:
        large-file.bin

See: `git lfs help smudge` for more details.

me@work MINGW64 ~/repros/large-file-lfs-3487/lfs-testing (large-file)
$ ls -lah large-file.bin
-rw-r--r-- 1 me 4096 1.0G Oct 25 19:58 large-file.bin

And when I clone this (in hindsight, I could have just created a simple local test repository...) with the Git built from your PR branch, it works!!! 🎉

me@work MINGW64 /usr/src/git/wip8 (huge-file-smudge-clean)
$ ./bin-wrappers/git -C ~/repros/large-file-lfs-3487/ clone lfs-testing test-with-fixed-git
Cloning into 'test-with-fixed-git'...
done.
Updating files: 100% (507/507), done.
Filtering content: 100% (505/505), 1.00 GiB | 13.17 MiB/s, done.
Encountered 1 file(s) that may not have been copied correctly on Windows:
        large-file.bin

See: `git lfs help smudge` for more details.

me@work MINGW64 /usr/src/git/wip8 (huge-file-smudge-clean)
$ ls -lh ~/repros/large-file-lfs-3487/test-with-fixed-git/large-file.bin
-rw-r--r-- 1 me 4096 5.0G Oct 26 09:20 /c/Users/johasc/repros/large-file-lfs-3487/test-with-fixed-git/large-file.bin

The only remaining issue is that Git LFS has no way of knowing that Git for Windows got the size right, and therefore still offers the warning that "1 file(s) [...] may not have been copied correctly" (emphasis mine).

[vtbassmatt]

[T]he Git built from your PR branch, it works!!!

🎉 thank you so much!

Git LFS has no way of knowing that Git for Windows got the size right, and therefore still offers the warning

I can't find the reference right now, but I believe the Git LFS project said they're willing to remove or alter that warning once Git for Windows no longer exhibits the issue. I'll put up a PR there once this one is on its way.

[dscho]

Git LFS has no way of knowing that Git for Windows got the size right, and therefore still offers the warning

I can't find the reference right now, but I believe the Git LFS project said they're willing to remove or alter that warning once Git for Windows no longer exhibits the issue. I'll put up a PR there once this one is on its way.

Great!

[Introduce a clean filter for extremely large files](/git-for-windows/git/pull/3487/commits/41739426ce3fb326643ec5067e57ff5753c1cab3)

One suggestion first: could you change that oneline to t1051: verify clean filter for extremely large files? Or maybe not... as I can report that this test case fails over here:

[...]
++ dd if=/dev/zero bs=1048576 count=5120
5120+0 records in
5120+0 records out
5368709120 bytes (5.4 GB, 5.0 GiB) copied, 22.9894 s, 234 MB/s
+++ wc -c
++ size=5368709120
++ test_config filter.checklarge.clean 'wc -c'
++ config_dir=
++ test filter.checklarge.clean = -C
++ test_when_finished 'test_unconfig  '\''filter.checklarge.clean'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig  '\''filter.checklarge.clean'\''
                } && (exit "$eval_ret"); eval_ret=$?; :'
++ git config filter.checklarge.clean 'wc -c'
++ echo 'big filter=checklarge'
++ git add big
++ git commit -m big
[master (root-commit) 482722f] big
 Author: A U Thor <[email protected]>
 1 file changed, 1 insertion(+)
 create mode 100644 big
++ rm big
++ git checkout -- big
+++ cat big
++ test 5368709120 -eq 1073741824
error: last command exited with $?=1
not ok 12 - files over 4GB convert on input
#
#         dd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) >big &&
#               size=$(wc -c <big) &&
#               test_config filter.checklarge.clean "wc -c" &&
#               echo "big filter=checklarge" >.gitattributes &&
#               git add big &&
#               git commit -m "big" &&
#               rm big &&
#               git checkout -- big &&
#               test "$size" -eq "$(cat big)"
#

So there is more work to do to make this test case pass. And when we know what this work is, we could put it into the same commit, keep the same oneline (i.e. the first line of the commit message), and then wordsmith the commit message a bit (I think we know what reviewers on the Git mailing list will say if a commit message refers to an earlier commit message for details... 😁).

[dscho]

So there is more work to do to make this test case pass.

The good news is that the equivalent worked when I tried it with Git LFS, which means that the process variant of git add is unaffected by the bug (or better put: side-stepped), unlike the non-process variant.