Skip to content
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

ls: force fetching metadata when called with -L -Z #4960

Merged
merged 2 commits into from
Jul 1, 2023

Conversation

granquet
Copy link
Contributor

@granquet granquet commented Jun 8, 2023

The metadata are not used but it permits to use the same path as the long format to ensure we return 1 on invalid symlinks when ls is invoked with ls -L -Z

should solve #2928

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the test and this the solution is good. I just think it should be somewhere else, namely in the get_security_context function. That function triggers the error if selinux is enabled so it should probably also trigger the error if it doesn't.

@@ -2238,6 +2238,7 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
for item in items {
let context_len = item.security_context.len();
longest_context_len = context_len.max(longest_context_len);
item.md(out);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a comment explaining why this is here would be nice. I'm also thinking maybe it makes more sense to fix this in the get_security_context function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at inserting something similar into get_security_context() but I really wanted to use the md() function from PathData as this is the "code path" that is taken in the case of ls -L -l and which generates the error.

At a first glance the patch looked a bit more complex to implement it inside get_security_context() as I don't have direct access to a PathData() or the out variable from get_security_context().

I can try to make it happen with a bit of rework though :)

Copy link
Member

@tertsdiepraam tertsdiepraam Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just attempting to dereference in the get_security_context function should work right?

"code path" that is taken in the case of ls -L -l

That's true, but this issue is about a different code path. The error in this path comes from get_security_context and retrieving the metadata is not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially what I'm proposing is this:

  • If security context is supported and the feature enabled: get the context and return the result
  • If not and must_dereference is true:
    • perform a deref, if it fails, return that error
    • return Ok("?".into())
  • Else just return Ok("?".into())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code to match your expectations a bit more closely, what about this version now?

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/ls/selinux-segfault is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link

github-actions bot commented Jun 9, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/ls/selinux-segfault is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good! I just think it needs a comment to explain why we do this. Feel free to mention me when it's done and I'll take another look :)

src/uu/ls/src/ls.rs Show resolved Hide resolved
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/ls/selinux-segfault is no longer failing!
Skipping an intermittent issue tests/rm/rm1

The metadata are not used but it permits to check the symlink is valid.
We then return 1 on invalid symlinks when ls is invoked with ls -L -Z

Signed-off-by: Guillaume Ranquet <[email protected]>
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/ls/selinux-segfault is no longer failing!

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It's all good now as soon as the CI is green.

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/du/apparent is no longer failing!
Congrats! The gnu test tests/ls/selinux-segfault is no longer failing!

@cakebaker cakebaker merged commit a145798 into uutils:main Jul 1, 2023
@cakebaker
Copy link
Contributor

Thanks for your PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants