Fix | vs. & logic error on check of retval from stat() - #406
Open
klausman wants to merge 2 commits into
Open
Conversation
Comparing this to the other stat() callsites (l.469 and l.734) made me wonder if `|` is actually the right operation here: we want to make sure the basic fields are there, but with `|` the inner part will always be nonzero (and thus `!()` will always be as-zero, defeating the entire check. I think this is a simple typo and should be `&` instead.
THinking this over some more, I relaized that the lgoic is not quite correct. As it is (ignoring the earlier `|` vs. `&` bug), we don't check if the fields we are about are _all_ there (we specifically care about e.g. st.stx_ino, st.stx_siz and st.stx_size); instead any of the budled fields in STATX_BASIC_STATS are sufficient to make the check pass. In reality, only some of the basic fields being there but not all is extremely unlikely if not impossible. Still, I think the code should reflect the intended logic, rather than a proximate check as it is now. So I fixed all three occasions of this check in file_scan.c.
Author
|
I realized there is another logic bug in these three checks, and fixed the main PR comment accordingly |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The three checks of
stat()return values have a logic bug in that they check if any of the basic stat fields (as bundled bySTATX_BASIC_STATS) are there, even though we care about multiple of them (e.g.st.stx_ino,st.stx_sizeandst.stx_mtime, possibly more).Fixed the logic check to ensure all basic fields are there (and folded in a smaller bug where
|was used when it should have been&).