Bug 650539
| Summary: | getfattr ignores NULs in output when using "text" encoding | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Paul Bolle <pebolle> | ||||||||
| Component: | attr | Assignee: | Kamil Dudka <kdudka> | ||||||||
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||||
| Severity: | medium | Docs Contact: | |||||||||
| Priority: | low | ||||||||||
| Version: | 13 | CC: | kdudka | ||||||||
| Target Milestone: | --- | ||||||||||
| Target Release: | --- | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Whiteboard: | |||||||||||
| Fixed In Version: | attr-2.4.44-4.fc13 | Doc Type: | Bug Fix | ||||||||
| Doc Text: | Story Points: | --- | |||||||||
| Clone Of: | |||||||||||
| : | 651119 (view as bug list) | Environment: | |||||||||
| Last Closed: | 2011-01-17 20:49:43 UTC | Type: | --- | ||||||||
| Regression: | --- | Mount Type: | --- | ||||||||
| Documentation: | --- | CRM: | |||||||||
| Verified Versions: | Category: | --- | |||||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||||
| Embargoed: | |||||||||||
| Bug Depends On: | |||||||||||
| Bug Blocks: | 651119 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Paul Bolle
2010-11-06 22:43:49 UTC
Created attachment 458391 [details]
Handle NULs in "text" encoded output
Created attachment 458849 [details]
extended version of the patch that handles embedded NULLs, too
Thank you for the bug report and the patch! It's indeed broken and your patch solves the problem. It stops on the first occurrence of '\0'. For some reason 'setfattr --restore' then appends the trailing zero automatically while operating in 'text' mode, and everything works fine in the case of SELinux attributes.
However, the encoding is still broken when embedded NULLs are involved:
$ touch foo
$ setfattr -n user.bar -v 'foo\000bar' foo
$ getfattr --dump -e hex foo
# file: foo
user.bar=0x666f6f00626172
$ getfattr --dump -e text foo | tee dump
# file: foo
user.bar="foo"
$ setfattr --restore dump
$ ./getfattr/.libs/getfattr --dump -e hex foo
# file: foo
user.bar=0x666f6f
It means we are still loosing information at this point ^^^. We should probably extend your patch this way:
$ setfattr -n user.bar -v 'foo\000bar' foo
$ getfattr --dump -e hex foo
# file: foo
user.bar=0x666f6f00626172
$ getfattr --dump -e text foo | tee dump
# file: foo
user.bar="foo\000bar"
$ setfattr --restore dump
$ ./getfattr/.libs/getfattr --dump -e hex foo
# file: foo
user.bar=0x666f6f00626172
Could you please have a look at the updated patch?
Actually, it reminded me an outstanding documentation bug (bug 587516), which is closely related to this one. Paul, could you please have a look at the following man page fix and check if the wording is clear to you? Thanks in advance! attachment #458858 [details] (In reply to comment #3) > Paul, could you please have a look at the > following man page fix and check if the wording is clear to you? One of these days, I hope. Please prod me if you get nervous. (In reply to comment #2) > We should probably extend your patch this way: > > $ setfattr -n user.bar -v 'foo\000bar' foo > $ getfattr --dump -e hex foo > # file: foo > user.bar=0x666f6f00626172 > > $ getfattr --dump -e text foo | tee dump > # file: foo > user.bar="foo\000bar" > > $ setfattr --restore dump > $ ./getfattr/.libs/getfattr --dump -e hex foo > # file: foo > user.bar=0x666f6f00626172 > > Could you please have a look at the updated patch? 0) I haven't looked at the patch yet, because I'm afraid that I need to think a bit about strings with embedded NULs. Those feel wrong to me. 1) A first reaction: if people want NULs to survive the tricks you use - which they seem not to do, otherwise they would have run into this issue before - they should just use the base64 encoding or the hex encoding. 2) But maybe it is specified somewhere what should be done here, making this discussion moot. Or perhaps there are competing implementations that can give some guidance here. (In reply to comment #5) > 0) I haven't looked at the patch yet, because I'm afraid that I need to think a > bit about strings with embedded NULs. Those feel wrong to me. s/strings/attributes/ > 1) A first reaction: if people want NULs to survive the tricks you use - which > they seem not to do, otherwise they would have run into this issue before - > they should just use the base64 encoding or the hex encoding. Dump/restore should either honestly fail, or work properly. If it silently looses information with zero exit code, then it needs to be fixed. I see the current behavior, even with your original patch applied, as a bug. > 2) But maybe it is specified somewhere what should be done here, making this > discussion moot. Or perhaps there are competing implementations that can give > some guidance here. Sure, I'll forward the issue upstream. Thanks for your comments. Created attachment 459025 [details]
[PATCH v2] proposed improvement of setfattr.1 man page
just fixed two typos...
Created attachment 459444 [details]
Trivial typo in setfattr.1
Just something I noticed when looking at this.
(In reply to comment #8) > Just something I noticed when looking at this. By the way, similar patch could be applied to getfattr.1 Comment on attachment 459444 [details] Trivial typo in setfattr.1 >-.SS OPTIONS >+.SH OPTIONS Thanks for review. I don't think it was a typo. ".SS" is a valid macro according to man(7) man page. .SS t Subheading t (like .SH, but used for a subsection inside a section). Its usage is fairly consistent across all attr/acl man pages. proposed upstream: http://lists.gnu.org/archive/html/acl-devel/2010-11/msg00001.html pushed upstream: http://git.savannah.gnu.org/cgit/attr.git/commit/?id=7fed444 Btw. I must admit you were right with the .SS/.SH usage -- the getfattr/setfattr man pages were indeed changed in the way you had suggested: http://git.savannah.gnu.org/cgit/attr.git/commit/?id=0a2d62b fixed in attr-2.4.44-6.fc15 attr-2.4.44-6.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/attr-2.4.44-6.fc14 attr-2.4.44-4.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/attr-2.4.44-4.fc13 attr-2.4.44-4.fc13 has been pushed to the Fedora 13 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update attr'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/attr-2.4.44-4.fc13 attr-2.4.44-6.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report. attr-2.4.44-4.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report. |