Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix archiving with console on SELinux: use ps x instead of ps ex #15508

Merged
merged 1 commit into from Aug 27, 2020
Merged

Fix archiving with console on SELinux: use ps x instead of ps ex #15508

merged 1 commit into from Aug 27, 2020

Conversation

ghost
Copy link

@ghost ghost commented Feb 3, 2020

Resolves ps errors when archiving as an account does not have access to /proc/ (typical on SELinux). Change 'ps ex' to 'ps x' for compatibility with SELinux.
Example of this issue in forum: https://forum.matomo.org/t/selinux-errors-about-ps/14540

@ghost ghost changed the title ps -x for compatibility with SELinux Fix: ps -x for compatibility with SELinux Feb 5, 2020
@ghost ghost changed the title Fix: ps -x for compatibility with SELinux Fix: Archiving with console on SELinux Feb 5, 2020
@ghost ghost requested a review from tsteur February 5, 2020 23:09
@tsteur
Copy link
Member

tsteur commented Feb 6, 2020

I'm not sure re possible consequences of that. Eg I reckon that for some users this could maybe break things where a different job runs under a different user/session?

Maybe someone knows a bit more. The only other thing I could imagine is to maybe first try ps -ex and if this doesn't work run ps -x maybe?

Maybe someone else knows more? @Findus23 maybe?

@tsteur tsteur added the Needs Review PRs that need a code review label Feb 10, 2020
@tsteur tsteur added this to the 4.0.0 milestone Feb 10, 2020
@diosmosis
Copy link
Member

I can't remember if this was the reason for ex, but years ago there was an issue on FreeBSD and to fix it we had to change the command. This might break for that or other systems.

@@ -242,7 +242,7 @@ private static function isProcFSMounted()

public static function getListOfRunningProcesses()
{
$processes = `ps ex 2>/dev/null`;
$processes = `ps x 2>/dev/null`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could do something like

$returnCode = null;
$processes = system(ps ex 2>/dev/null, $returnCode);
if ($returnCode != 0) {
     $processes = system(ps x 2>/dev/null, $returnCode);
}

? And same below?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running -ex on secure linux increases the size of the audit log. It is a policy violation. Perhaps run 'x' first? Least privileged first?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could do this? Though if ps ex fails for a temporary reason and ps x fails because the OS is wrong that could result in some strange error messages. Would be a bit of an edge case though. Probably better than trying to detect the correct system/command to use. (I guess we could also allow the command options to be specified through config or something.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leftmostlane sorry for getting back to you so late. Any chance you could look at the approach above where we fall back to ps x if ps ex doesn't work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running 'ex' on secure Linux increases the size of the audit log and is a policy violation. On shared systems this fills up the audit log with access violations. Perhaps run 'x' first? Test for least privileged first then fall back to 'ex' if that does not work.

$returnCode = null;
$processes = system(ps x 2>/dev/null, $returnCode);
if ($returnCode != 0) {
     $processes = system(ps ex 2>/dev/null, $returnCode);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I did some various tests on different OS and some more research and it should be fine to use only x.

I also went back the history (took a while) and noticed I had added ps -e about 7 years ago in 113e403#diff-daeea28082bcbb1d6765684dc7f99fb2R92

It was initially -e as in "Select all processes.".

Then it became ps -ex.

And then it became ps ex in ca84f22

Which I think changes the meaning from select all processes to Show the environment after the command.. However, we don't need the environment. I think it was meant to be changed to ps -e x if that makes any sense and I understand things correctly.

The referenced issue says

On some systems the command 'ps -e' will not return processes that are run via cron (since they are not attached to a terminal). This causes the ./console climulti:request command to abort early and return no data, which in turn causes ./console core:archive to fail.

Which caused the change from ps -ex to ps ex. Unfortunately the referenced forum link does no longer work.

There is also #7054 which suggests to use ps -e.

I'm planning to merge this PR as is within the next 7 days or so. Hoping it also fixes things on solaris maybe.

Otherwise we'd need to do things like php_uname () and adjust the command based on the OS.

If anyone has some further insights be great to let me know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great!

@tsteur tsteur changed the base branch from 3.x-dev to 4.x-dev August 27, 2020 21:50
@tsteur tsteur merged commit 54544e8 into matomo-org:4.x-dev Aug 27, 2020
@mattab mattab changed the title Fix: Archiving with console on SELinux Fix archiving with console on SELinux: use ps x instead of ps ex Sep 29, 2020
@mattab mattab added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants