search

containerd / continuity

A transport-agnostic, filesystem metadata manifest system
https://containerd.io
Apache License 2.0
139 stars 67 forks source link

Fix copy_file_range usage for files > 2GB on 32-bit archs #122

Closed poizan42 closed 5 years ago

poizan42 commented 6 years ago

So this turned out to actually have nothing to do with MIPS, but just 32-bit architectures in general.

On mips copy_file_range actually fails with EINVAL for sizes over 2GB.

This fix ensures that we are never trying to copy more than 2GB at a time and at the same time avoids overflows when the file is larger than 4GB.

Note that when I tried building in a qemu vm I had to increase the test timeout because it took more than 10 minutes to run TestCopyWithLargeFile (in fs).

poizan42 commented 6 years ago

Updated to just ensure it doesn't exceed SSIZE_MAX (which corresponds to max int in go). Note that this check is a no-op on 64-bit systems.

poizan42 commented 6 years ago

I should remember to actually include the updated file...

dmcgowan commented 6 years ago

LGTM

cpuguy83 commented 6 years ago

Are we sure this fixes it? IIRC the way the sys call gets made this ends up converting to 2GB anyway.

poizan42 commented 6 years ago

@cpuguy83 As far as I can tell it is up to the fs implementation. Watch for yourself, the syscall entrypoint is here which calls vfs_copy_file_range with copying in/out of the offsets. It then verifies the buffer/length with rw_verify_area which checks that it does not exceed max ssize_t (which is what caused this to fail on 32-bit systems).

So on 64-bit systems where ssize_t is int64 this succeeds for sizes > 2GB. Then it uses clone_file_range if the fs supports that and if not it tries copy_file_range. It is only if the fs supports neither that we hit the do_splice_direct call with a max size of MAX_RW_COUNT which limits the size to a bit below 2GB (but doesn't fail in that case if the size is larger).

AkihiroSuda commented 5 years ago

What's current status? Is this mergable? @cpuguy83 @dmcgowan @stevvooe

cpuguy83 commented 5 years ago

Should be. I don't have write access here.

AkihiroSuda commented 5 years ago

Updated to just ensure it doesn't exceed SSIZE_MAX (which corresponds to max int in go). Note that this check is a no-op on 64-bit systems.

@poizan42 Could you add this comment to the code? then LGTM

estesp commented 5 years ago

@poizan42 can you add a comment per @AkihiroSuda's request? Maybe something straight from your commit at the top of the if size > 0 block?

Something like: Ensure that we are never trying to copy more than SSIZE_MAX at a time and at the same time avoids overflows when the file is larger than 4GB on 32-bit systems.

This looks worth getting in; happy to merge after getting a comment added.

AkihiroSuda commented 5 years ago

ping

poizan42 commented 5 years ago

CI fails with

 1f02a3c - FAIL - commit subject exceeds 90 characters

Isn't the "commit subject" the first line of the description? It is only 59 characters long.

estesp commented 5 years ago

@poizan42 in git, until it finds a blank line, everything up to that point is the "subject"/title: The text up to the first blank line in a commit message is treated as the commit title, and that title is used throughout Git...

You need a blank line after your 59 character title to fix the CI check.

poizan42 commented 5 years ago

This should be ready to be merged now