ARM-software
Fix sudo
Fixes https://github.com/ARM-software/devlib/issues/703
@marcbonnici Can you remember why we used a one-space prompt with sudo -p ' ' instead of a completely empty prompt ? This prompt seems to be picked up in stdout or stderr when using Target.execute(), modifying the return value as a result.
It seems that empty prompt used to not work at the time of Ubuntu 18.04: https://serverfault.com/questions/967859/disable-sudo-prompt-without-disabling-the-need-for-sudo-password
I'll try to find what version is the cutoff date
It works on Ubuntu 20.04 at least, so the cutoff date is earlier than that.
EDIT: tested on docker with Ubuntu 14.04 and it works as well. So either there was a bug in a specific version of sudo, or the issue manifested itself in another distro
EDIT 2: The original -p ' ' code was added in commit 1381944e5bd70658c62e461bd7266404a6dfed86. The reason a space prompt was used is because sudo on WSL was broken in 2020.
@marcbonnici Do you have access to a WSL install to see if that is still a problem nowadays ? There seems to be a WSL 2 environment that uses a real linux kernel so hopefully it would just ship a regular sudo somehow
Just checked and I can confirm on my WSL 1 environment the bug is still present, however it does appear to be fixed when switching to WSL2.
Just to try and understand better, why was the additional space added causing a breakage here? Could the check to determine whether there are additional messages printed not ignore the single additional space?
However worst case, if we wanted to try and maintain compatibility with WS1 maybe we could default to the no prompt option and if this fails, try the spaced version before bailing?
Just checked and I can confirm on my WSL 1 environment the bug is still present, however it does appear to be fixed when switching to WSL2.
Thanks for checking, that's partly good news. Apparently WSL 1 is not dead though ... https://learn.microsoft.com/en-us/windows/wsl/faq#what-will-happen-to-wsl-1--will-it-be-abandoned-
Just to try and understand better, why was the additional space added causing a breakage here?
execute() is supposed to be returning stdout + stderr. With as_root=True, it did not: it returned it with an extra space in front. As a baseline, having a different output just because we are root is not really acceptable for a command that is otherwise completely unaffected by permissions. For example we routinely end up adding an as_root=True to some commands that missed them, without necessarily revalidating everything. On top of that, we may not even be using sudo in some cases if we are already root AFAIR, so as_root=True may or may not add a space to the output.
I discovered this when modifying check_connection() to ensure that a command with no stdout and no stderr content actually returns an empty string (which was not the case, both because of that prompt and because of the password expiry printed by the PAM module in my case).
However worst case, if we wanted to try and maintain compatibility with WS1 maybe we could default to the no prompt option and if this fails, try the spaced version before bailing?
Honestly I don't think it's a good idea considering the above. The only other alternative I see is to postprocess the output to remove the prompt in case sudo was used (and precisely sudo, not just as_root=True), but if no prompt works that's even better.
Thanks for the information.
Thanks for checking, that's partly good news. Apparently WSL 1 is not dead though ...
I see, I'm not sure if there are still any requirements to use WSL1 over WSL2, I do vaguely recall there being some issues with WSL2 but these could have been fixed by now.
Honestly I don't think it's a good idea considering the above. The only other alternative I see is to postprocess the output to remove the prompt in case sudo was used (and precisely sudo, not just as_root=True), but if no prompt works that's even better.
Makes sense, I can't think of another option to maintain the compatibility other than post processing either. The only other options would be to require the use of WSL2 but that also wouldn't be ideal if WSL1 is being kept around.
Do we actually care about WSL at all ? If not, it could make sense to just use -p '', and if one day we do want to add explicit support for it, we can revisit and add the post processing where needed, especially if the error from sudo is obvious.
That's a fair comment, I guess the real world scenarios where the target platform is required to be WSL(1) is likely very limited. Just for the background, the issue was originally found when running the devlib test suite from WSL (which creates a localtarget) as opposed to an actual use case.
As you say, perhaps just documenting this issue might be the cleaner option.
@marcbonnici Is there a specific place you'd see to document that ? I don't think there is any list of officially supported targets in devlib ATM
True... Looks like we don't really have a good place for this in the current documentation. Perhaps we might need to make do with some comments in the code next to the default prompt and a summary of this discussion in the commit message so at least if we need to revisit this in the future there is a simple way to determine a potential issue and what a solution could be?
PR updated with inline comments and more details in the commit message