Remove posix_getpwuid and compare only userid

[hoellen]

I had some trouble with the command occ and the cron job freezing and causing a thread to load 100% infinitely.
This patch fixed it for me.

posix_getpwuid returns an array with information about user/group/etc. for current user. These information is not available when /etc/passwd is not readable (see https://secure.php.net/manual/en/function.posix-getpwuid.php#45994).

Calling posix_getpwuid() twice should not result in something different for the same input, and if it is not the same input, the result is also never the same. So comparing only the user id should be fine.

Same as #11091 for console.php and cron.php.

[kesselb]

Thanks 👍

-u user' The -u (user) option causes sudo to run the specified command as a user other than root. To specify a uid instead of a user name, #uid. When running commands as a uid, many shells require that the '#' be escaped with a backslash ('\').

https://linux.die.net/man/8/sudo

[hoellen]

Thanks @kesselb, fixed 👍

[Spilarix]

Thanks for your PR.
It also fixes some problems when using the docker version of nextcloud with a non-root user and php 7.4.
However, there are 2 references to posix_getpwuid in getForm() that you are not taking in charge : https://github.com/nextcloud/server/blob/master/apps/settings/lib/Settings/Admin/Server.php
Variables created are used in https://github.com/nextcloud/server/blob/master/apps/settings/templates/settings/admin/server.php line 101 and 102.

Could you add them to your pull request please ?

[hoellen]

I rebased and also removed the posix function from the settings app (thanks @Spilarix). @kesselb can you please review again?

[MorrisJobke]

I can understand the idea behind this, but it then reduces the UX for admins. Now a numerical value is shown as a hint in the setup checks. Isn't it possible to get the user name somehow to properly show it in the UI?

[Spilarix]

I can understand the idea behind this, but it then reduces the UX for admins. Now a numerical value is shown as a hint in the setup checks. Isn't it possible to get the user name somehow to properly show it in the UI?

I don't think so because in this case, /etc/passwd is not readable at all.

[kesselb]

• Check if function posix_getpwuid exist
• If yes execute the function
• Show the username if response is not false otherwise the uid

[Spilarix]

@hoellen Will you take these changes in charge or do you need help ?

[hoellen]

If yes execute the function

If I execute the function, the PHP script freezes and fully loads a thread. I believe this is some misconfiguration on my side. I still need to investigate it.

@hoellen Will you take these changes in charge or do you need help ?

I have no time this week to do this. Feel free to take over, if it is urgent for you.

[Spilarix]

If yes execute the function

If I execute the function, the PHP script freezes and fully loads a thread. I believe this is some misconfiguration on my side. I still need to investigate it.

@hoellen Does it freeze when the call to posix_getpwuid return false or whatever its value ?

[kesselb]

@MorrisJobke from my POV this is good to merge without the third commit. Mind to drop the third commit and merge it?

[MorrisJobke]

@hoellen I opened a PR with only the first 2 commits in #23436 (review)

I you want to get this for your Hacktoberfest count, then feel free to open a new PR with only the two commits so it counts towards your PR number. 😃

[Spilarix]

@hoellen I opened a PR with only the first 2 commits in #23436 (review)

I you want to get this for your Hacktoberfest count, then feel free to open a new PR with only the two commits so it counts towards your PR number. 😃

@MorrisJobke Is it possible to include this in a 20.0.X release instead of 21 ?

[MorrisJobke]

@hoellen I count your 🚀 reaction to my comment as "ship it" 😄

[MorrisJobke]

#23436 is merged -> let's close this here.

[MorrisJobke]

@hoellen I opened a PR with only the first 2 commits in #23436 (review)
I you want to get this for your Hacktoberfest count, then feel free to open a new PR with only the two commits so it counts towards your PR number. 😃

@MorrisJobke Is it possible to include this in a 20.0.X release instead of 21 ?

I would not do it for now. Is this causing that much trouble?

[Spilarix]

@MorrisJobke Is it possible to include this in a 20.0.X release instead of 21 ?

I would not do it for now. Is this causing that much trouble?

@MorrisJobke It's a fix and not an evolution, that's the first reason why it seems strange to me to not get it before 21.x.
Maybe the choice of version is made on another criteria ?
To answer more precisely, in my case with docker, logs are full of these errors because calls are made very frequently.
I would like to fix this problem to be able to look logs without "pollution" and avoid useless I/O on disk causing performance issues / premature wear.
In the case of @hoellen : "I had some trouble with the command occ and the cron job freezing and causing a thread to load 100% infinitely".
That's why it would be really usefull to get it in 20.x ! 🙏

[hoellen]

@hoellen I count your rocket reaction to my comment as "ship it" smile

Yes, thats right. Thanks 😄

I would not do it for now. Is this causing that much trouble?

I fully understand @Spilarix opinion, but for me: I will just stick to PHP 7.3 until Nextcloud 21. So its fine.