New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Fseek for offset > 2GiB #1381
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fseek() is modelled after fseek(), not lseek(), and intended to return an int.
You're totally right that there are bugs though, lseek() return should not be used directly but instead converted to something like return (lseek(...) == -1 : -1 : 0)
ca45e02
to
4124cee
Compare
I've omitted the changes to the call sites because Fseek can also return -2 if not implemented. I think this is quite academic here - there's little chance of a user facing choice for compressed vs uncompressed IO. |
Please leave out the return codes documentation because that only makes the broken -2 return documented and stand out for as something special and supported for Fseek() when it can happen for pretty much anything for all sorts of arbitrary reasons. I think we should instead fix all those cases to return -1 with errno set (or something) instead. That is obviously beyond the scope of this PR. |
This code eliminates a false positive failure when the destination position is > 2GiB. This is done by changing the contract for `Fseek`. Now it returns `0` on success instead of an `int` offset. Care should be used to interpret the result as there is a difference in semantics between the POSIX `fseek(2)`. Existing code is correct: negative results are still failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Third time the charm... Thanks for the patch and perseverance! 😅
Hmm, for some reason CI is refusing to run for this. I'll open a separate PR for this, there doesn't seem to be any way to force it through here. No clue... |
Merged via #1390, no idea why this wasn't running the CI. Anyway, thanks again. |
rpmio wraps a number of different file types, regular files and ones with compression. The only file type that supports seeking is regular files. This uses
LSEEK(2)
under the hood. I believe this is defined to returnoff_t
which is signed. The function returns the offset (which is non negative in the normal case), or-1
on error.There's two specific issues here, so two patches:
off_t
is being truncated toint
(usually 32bit) on 64 bit arch. As there's only one implementation we can change the signature. Also the Ftell returnsoff_t
already so this simply brings it in line.The error checks are using< 0
which is strictly correct - it's impossible to return a normal offset that's negative. Still, the function is documented as returning-1
so in the interest of correctness, I think it's worth fixing that too.