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

[master] Fix issue 63779: Handle xattr.read unicode errors by mimicking builtin xattr #64039

Merged
merged 8 commits into from
May 3, 2023

Conversation

sheagcraig
Copy link
Contributor

@sheagcraig sheagcraig commented Apr 7, 2023

What does this PR do?

Apple puts bytes which are not decode-able into some common xattrs. This can be verified by downloading just about any app (from the web or the app store) and using the builtin /usr/bin/xattr /path/to/app on it and then attempting the same thing with Salt (salt-call xattr.list /path/to/app).

If you look at the actual bytes, it's tempting in some cases to try to parse them; often they are binary plists. However, these fail to parse with the builtin python plistlib (which does handle binary plists now). I tried it, they don't parse with NSPropertyListSerialization either, and even if they did, Salt for Mac no longer includes PyObjC so I'm not sure that would be helpful.

Regardless, I've seen other xattrs in the wild which are not binary plist and also fail when trying to decode to Unicode.

What issues does this PR fix or reference?

Fixes: #63779

Previous Behavior

Raises CommandExecutionError

New Behavior

Uses the bytes.decode argument errors="replace" to sub in � for anything that fails decoding. (This is what the builtin xattr tool does on output as well).

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Uses `bytes.decode` argument `errors="replace"`
@sheagcraig sheagcraig requested a review from a team as a code owner April 7, 2023 11:33
@sheagcraig sheagcraig requested review from garethgreenaway and removed request for a team April 7, 2023 11:33
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Fix issue 63779: Handle xattr.read unicode errors by mimicking builtin xattr [master] Fix issue 63779: Handle xattr.read unicode errors by mimicking builtin xattr Apr 7, 2023
@sheagcraig
Copy link
Contributor Author

I'm working on the test. The existing integration test I imagine won't work since I can't write bytes using the function, so I have to start a unit test from scratch.

@sheagcraig
Copy link
Contributor Author

Okay-test is in and this should be good to go. Also cut #64041 to the 3006.x branch.

@sheagcraig
Copy link
Contributor Author

@garethgreenaway anything I need to do to get this merged as well? Thanks!

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.

[BUG] mac_xattr execution module fails on binary plist attributes
2 participants