Skip to content

Exit code equals 1 if configuration file is not owned by root#508

Closed
sblondon wants to merge 3 commits intologrotate:masterfrom
sblondon:exit_code_for_error
Closed

Exit code equals 1 if configuration file is not owned by root#508
sblondon wants to merge 3 commits intologrotate:masterfrom
sblondon:exit_code_for_error

Conversation

@sblondon
Copy link
Contributor

@sblondon sblondon commented Apr 23, 2023

The PR 448 let me think the error messages strategy is:
MESS_ERROR if the error stops logrotate. So exit value should not be 0.
MESS_WARNING if it's possible to continue to run logrotate. Exit value is 0.

Currently, if configuration file is not owner by root, logrotate exits with 0 as exit code (it's in confic.c ):

message(MESS_ERROR,
    "Ignoring %s because the file owner is wrong (should be root or user with uid 0).\n",
    configFile);
$ sudo LANG=C ./logrotate logrotate.not_root.conf
error: Ignoring logrotate.not_root.conf because the file owner is wrong (should be root or user with uid 0).
$ echo $?
0

This PR changes the exit code to 1.

I found two cases where it should use MESS_WARNING because configuration lines are ignored:

  1. lines 342-343:
message(MESS_ERROR, "%s:%d extra arguments for "
    "%s\n", configFile, lineNum, directive); 
$ sudo LANG=C ./logrotate logrotate.extra_argument.conf 
error: logrotate.extra_argument.conf:2 unknown option 'extraarg' -- ignoring line
$ echo $?
0
  1. lines 1746-1747:
message(MESS_ERROR, "%s:%d unknown option '%s' "
    "-- ignoring line\n", configFile, lineNum, key);
$ sudo LANG=C ./logrotate logrotate.invalid_directive.conf
error: logrotate.invalid_directive.conf:2 unknown option 'invaliddirective' -- ignoring line
$ echo $?
0

I can add them to the PR if you think it's the correct behavior.

Tests are all green. I didn't understand how to modify them to add exit value assertion and didn't find documentation about them. So, there is no additional test.

kdudka
kdudka requested changes Apr 24, 2023
View reviewed changes
Copy link
Member

@kdudka kdudka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think it makes sense to turn the other two errors into warnings, as you say.

As for checking logrotate's exit code in tests, you can have a look at these examples:

@sblondon
Copy link
Contributor Author

sblondon commented Apr 25, 2023 •
edited
Loading

I changed the two MESS_ERROR to MESS_WARN you agreed.

I did not add tests because the code has to be runned by root due to the condition if (getuid() == ROOT_UID) (https://github.com/logrotate/logrotate/blob/master/config.c#L1054). It can be changed by modifying AC_DEFINE_UNQUOTED([ROOT_UID], [0], [Root user-id.]) in configure.ac but it requires to recompile logrotate to run the test. Do you see a better way to do it?

kdudka
kdudka approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

Thanks for the update! As for the tests, one could instrument the call to getuid() at run-time. But let's not overcomplicate it.

@sblondon
Copy link
Contributor Author

sblondon commented Apr 25, 2023

Thanks for the update! As for the tests, one could instrument the call to getuid() at run-time. But let's not overcomplicate it.

Ok.
Do you want I add a line in ChangeLog.md file about this PR? Or it's good enough to be merged?

@kdudka
Copy link
Member

kdudka commented Apr 25, 2023

No worries. I will update the change log in a batch before the next release. I plan to merge this in a few days unless there is any more discussion. Thanks for the contribution!

cgzones
cgzones reviewed Apr 28, 2023
View reviewed changes
@kdudka kdudka closed this in 99b9b29 May 9, 2023
kdudka pushed a commit that referenced this pull request May 9, 2023
@sblondon @kdudka
exit code equals 1 if configuration file is writable by group or other
324a752 
Closes: #508
kdudka pushed a commit that referenced this pull request May 9, 2023
@sblondon @kdudka
use MESS_WARN for invalid line in configuration file
7029565 
Closes: #508
@sblondon sblondon deleted the exit_code_for_error branch May 10, 2023 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kdudka kdudka kdudka approved these changes

@cgzones cgzones cgzones approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sblondon @kdudka @cgzones