search

Kiwi / clyde

Next-generation libalpm/makepkg wrapper.
https://kiwi.github.io/clyde
Other
63 stars 8 forks source link

Handle missing user in /etc/password when building #133

Closed sebnow closed 13 years ago

sebnow commented 13 years ago

The getpwnam() function returns NULL if a user is not found in /etc/password, but errno will not be set. In this case nil is returned, and an exception is thrown.

The patch adds handling for this edge case. A warning will be printed that ownership can not be changed for the build directory, as this is not a fatal error.

sebnow commented 13 years ago

The bug was reported on the Archlinux forums. To replicate the bug simple change BuildUser in the config to a non-existant user and attempt to build something from AUR.

juster commented 13 years ago

I see that the error handling I did is sloppy and would like to fix that. This looks pretty good, but I don't see why an invalid builduser provided by the user shouldn't be an error?

sebnow commented 13 years ago

My reasoning was that it wouldn't prevent the build from succeeding, and since the user has root privileges (I think), they can fix the permissions.

Thinking about it afterwards, maybe it should be an error. Making it a warning doesn't fix the poster's (on the BBS) issue. I don't quite understand why that setting is system-wide though. Chowning to a different user than is running Clyde is just as wrong as chowning to root, unless it's also a user setting?

On 22/06/2011, at 22:45, justerreply@reply.github.com wrote:

I see that the error handling I did is sloppy and would like to fix that. This looks pretty good, but I don't see why an invalid builduser provided by the user shouldn't be an error?

Reply to this email directly or view it on GitHub: https://github.com/Kiwi/clyde/pull/133#issuecomment-1418468

juster commented 13 years ago

My opinion on the BBS bug is that the larger problem is the BuildUser set in clyde.conf did not exist. That is what is causing that person's error. An error message stating this, similar to what you have, would be preferable to how things work now. Right now it just blows up and the user doesn't (didn't) understand that the BuildUser they were using was the name of a user that plain didn't exist. All the user would have to do is set BuildUser to a proper value or remove it entirely if using sudo.

The setting is not necessarily system-wide. If the BuildUser is not set in clyde.conf, clyde uses the SUDO_USER environment variable, or it finally gives up and uses root. See get_builduser() in aur.lua. Maybe clyde should give highest priority to SUDO_USER, I dunno. You must keep in mind however, we are not just chowning to a different user all the time. We are chowning to a different user solely when being run as root. This is tailored for when we call clyde with sudo or su.

Anyways this was mostly rambling. I will will merge your patch. Thanks for the code! I might change a few things though, like erroring out. Though maybe not in this location... Is that okay?

sebnow commented 13 years ago

Yeah, no problem. Wasn't sure how Clyde does error reporting to the user anyway :).

On 23/06/2011, at 11:50, justerreply@reply.github.com wrote:

My opinion on the BBS bug is that the larger problem is the BuildUser set in clyde.conf did not exist. That is what is causing that person's error. An error message stating this, similar to what you have, would be preferable to how things work now. Right now it just blows up and the user doesn't (didn't) understand that the BuildUser they were using was the name of a user that plain didn't exist. All the user would have to do is set BuildUser to a proper value or remove it entirely if using sudo.

The setting is not necessarily system-wide. If the BuildUser is not set in clyde.conf, clyde uses the SUDO_USER environment variable, or it finally gives up and uses root. See get_builduser() in aur.lua. Maybe clyde should give highest priority to SUDO_USER, I dunno. You must keep in mind however, we are not just chowning to a different user all the time. We are chowning to a different user solely when being run as root. This is tailored for when we call clyde with sudo or su.

Anyways this was mostly rambling. I will will merge your patch. Thanks for the code! I might change a few things though, like erroring out. Though maybe not in this location... Is that okay?

Reply to this email directly or view it on GitHub: https://github.com/Kiwi/clyde/pull/133#issuecomment-1422852