FS#29769 - [backuppc] Init script fails silently
Attached to Project:
Community Packages
Opened by Jacob Joseph (jacobjjoseph.org) - Saturday, 05 May 2012, 20:13 GMT
Last edited by Sébastien Luttringer (seblu) - Thursday, 10 May 2012, 21:49 GMT
Opened by Jacob Joseph (jacobjjoseph.org) - Saturday, 05 May 2012, 20:13 GMT
Last edited by Sébastien Luttringer (seblu) - Thursday, 10 May 2012, 21:49 GMT
|
Details
The backuppc init script fails to start when the
configuration file contains errors, and does not print the
errors. They are not logged, either. The issue is that the
init script does not print the errors, due to an
over-aggressive "&> /dev/null" after the
start-stop-daemon. Only after removing this was I able to
debug the actual issue. (It turns out that I had configured
a mistaken $Conf{SendmailPath} = '').
I believe the redirection to /dev/null should be removed. The start-stop-daemon --quiet should be sufficient. If there are messages that one would prefer not be printed as a matter of course, pipe through a "grep -v" for each of them. I find it frustrating when the init script gets in the way of a program that has otherwise fine output. (More generally, and too much so for this bug, is there a policy in Arch about handling output from programs in init scripts?) Thanks much. ~Jacob |
This task depends upon
Closed by Sébastien Luttringer (seblu)
Thursday, 10 May 2012, 21:49 GMT
Reason for closing: Fixed
Additional comments about closing: fix in backuppc-3.2.1-6. Thanks to Dave Reisner.
Thursday, 10 May 2012, 21:49 GMT
Reason for closing: Fixed
Additional comments about closing: fix in backuppc-3.2.1-6. Thanks to Dave Reisner.
Use of qw(...) as parentheses is deprecated at /usr/share/backuppc/lib/BackupPC/Storage/Text.pm line 301.
Use of qw(...) as parentheses is deprecated at /usr/share/backuppc/lib/BackupPC/Lib.pm line 1412.
About grep, it's not serious. The best way to avoid those messages would to provide a patch upstream.
I agree we should remove redirection when those lines are gone.
You're right that the two deprecation messages could be a slight nuisance. However, I believe removing *ALL* useful output on account of a two verbose warnings is misguided. To me, it is a bug when the init script does not behave as intended, which is the case here, and the reason for my filing.
I propose that this script be fixed by purposely ignoring the messages:
2>&1 | grep -v "parentheses is deprecated"
In any case, the redirection should be removed. It masks meaningful errors.
An upstream fix is likely warranted, too.
~Jacob
A lot of initscripts redirect stdout and stderr to dave as daemons have to drop those fd to be properly daemonized.
Daemons must remove their links to its launching console. Here you suggest to link it to a shell pipe.
Besides that, using your grep suggestion, initscripts always return a failure (i don't look why).
In you case, you make mistake with configuration and initscripts show you a failure. This is the expected behaviour.
As many daemon debugging you should look into his log file (/var/log/backuppc/LOG) to fix you issue, or start it manually disabling daemon.
You can also look suggestion from systemd about this.
Jacob, the best way to cleanly remove those redirection (when daemon is not started with @) is to report this issue upstream and have a fix.
http://sourceforge.net/mailarchive/forum.php?thread_name=4EEE5465.5050603%40unige.ch&forum_name=backuppc-users
Also, I don't quite follow your comment about daemons disconnecting from the console stdout/stderr. BackupPC, of course, does disconnect from STDIN et. al, but only after potentially printing some other very important messages. Consider:
-------------------------
sub Main_Initialize
{
umask($Conf{UmaskMode});
#
# Check for another running process, verify executables are configured
# correctly and make sure $TopDir is on a file system that supports
# hardlinks.
#
if ( $Info{pid} ne "" && kill(0, $Info{pid}) ) {
print(STDERR $bpc->timeStamp,
"Another BackupPC is running (pid $Info{pid}); quitting...\n");
exit(1);
}
foreach my $progName ( qw(SmbClientPath NmbLookupPath PingPath DfPath
SendmailPath SshPath) ) {
next if ( $Conf{$progName} eq "" || -x $Conf{$progName} );
print(STDERR $bpc->timeStamp,
"\$Conf{$progName} = '$Conf{$progName}' is not a"
. " valid executable program\n");
exit(1);
}
if ( !$bpc->HardlinkTest("$TopDir/pc", "$TopDir/cpool") ) {
print(STDERR $bpc->timeStamp, "Can't create a test hardlink between a file"
. " in $TopDir/pc and $TopDir/cpool. Either these are different"
. " file systems, or this file system doesn't support hardlinks,"
. " or these directories don't exist, or there is a permissions"
. " problem, or the file system is out of inodes or full. Use"
. " df, df -i, and ls -ld to check each of these possibilities."
. " Quitting...\n");
exit(1);
}
if ( $opts{d} ) {
#
# daemonize by forking; more robust method per:
# http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=301057
#
my $pid;
defined($pid = fork) or die("Can't fork: $!");
exit if ( $pid ); # parent exits
POSIX::setsid();
defined($pid = fork) or die("Can't fork: $!");
exit if $pid; # parent exits
chdir ("/") or die("Cannot chdir to /: $!\n");
close(STDIN);
open(STDIN , ">/dev/null") or die("Cannot open /dev/null as stdin\n");
# STDOUT and STDERR are handled in LogFileOpen() right below,
# otherwise we would have to reopen them too.
}
#
# Open the LOG file and redirect STDOUT, STDERR etc
#
LogFileOpen();
....
--------------------------------------------
I argue that dropping all output to /dev/null can never be an appropriate solution for an init script.
~Jacob
I doesn't see any patch in backuppc-devel which i can include prior to next release.
> but only after potentially printing some other very important messages.
Warnings are not important message.
> I argue that dropping all output to /dev/null can never be an appropriate solution for an init script.
Never? Cons-example: Look the code of parallel daemons (@) in our initscripts project.
Jacob, we agree together, this redirection should not be here. Let's be consistent, either all outputs are removed or nothing is removed.
We don't want those useless warnings, so all is removed. Please provide a patch to fix the warning display, and i will drop the redirection.
Cheers,
>Warnings are not important message.
All of the messages described in the opening stanza of the BackupPC code I pasted are definitely not warnings. These are all followed by exit(1). They are absolutely important. Further, the error that prompted me to open this bug was not reported, as it should have been.
We agree to a point. However, I believe the position you are taking is in direct contrast to the "Code Correctness over Convenience" clause of the Arch Way document: https://wiki.archlinux.org/index.php/The_Arch_Way. Sure, redirecting to /dev/null may be a quick way to get rid of a few harmless warnings, but it masks important behavior--behavior that prevented me from understanding the function of the script. No, I don't agree that this should be debugged by skipping the init script, save as a very last resort. I encourage you to remove the redirection now, and resolve the warnings if need-be in a more robust manner.
~Jacob
Here we agree on:
- remove redirection
- remove warnings prints
So please, would you help archlinux to fix this by providing a patch for warnings or deal with upstream to do it.
http://svnweb.mageia.org/packages/cauldron/backuppc/current/SOURCES/BackupPC-3.2.1-CVE-2011-170886.diff?view=markup