search

landley / toybox

toybox
http://landley.net/toybox
BSD Zero Clause License
2.44k stars 340 forks source link

Fix tests for getfattr/setfattr #482

Closed tweksteen closed 9 months ago

tweksteen commented 9 months ago

The test system may have extended attributes set by default (for instance, security.selinux on Android). Support these by only testing part of the output.

This is similar to tests/chattr.test which filters the output.

tweksteen commented 9 months ago

@enh-google fyi

landley commented 9 months ago

I don't have much domain expertise here, and am not sure what we should be testing. You're filtering out everything except comment lines and user keys, instead of filtering out known bad stuff. I take it various security blankets can add just random crap anywhere uncontrollably? This is a better approach than using the -n option? (The reason we don't need to | sort the output because there's a qsort() in getfattr.c...)

tweksteen commented 9 months ago

That's right, the kernel may add some of these labels depending on the system configuration (that's why just filtering out security.selinux might not be robust enough).

I'm not sure how much coverage you preferred, but yes the -n option is more precise. I've updated the commit to use it in all test cases. Thanks.

enh-google commented 9 months ago

that seems like a bad idea? we already had tests for -n, but now we don't have tests for the non--n path? (personally i'd have just filtered out the security cruft. realistically i don't think we're going to see others; smack just didn't get the same traction afaict, and even if it did, that's just one more thing to filter out. filtering also matches what we've done in similar cases in other tests...)

tweksteen commented 9 months ago

There is security.ima that could realistically be in use? From reading xattr(7) man page, I think other namespaces are unlikely to be populated. Maybe we could filter out anything in the security namespace (that is, matching security.)?

enh-google commented 9 months ago

yes, that's even better if others are already using the security. namespace!

tweksteen commented 9 months ago

I didn't realise the previous version of the commit got merged. Just in case, there is an updated version following the conversation above. Thanks.

landley commented 9 months ago

First time I've applied the same pull request twice. Commit f79b72761f49