Bug#1051805: systemd-cron: shell redirect not working in crontab entries

[наб]

Control: tag -1 + confirmed upstream

On Tue, Sep 12, 2023 at 10:11:58PM +0200, Alexandre Detiste wrote:
> In v1.x the trailing "> /dev/null" was purposely chopped of the end of the line
> because there wasn't an OnSucces= handler anyway and the goal
> was to further parse the bash-one liner.
> 
> Some of the brittle bash-parsing has already been dropped of from 2.x.
Concretely, we already killed stuff like this:
        if (len(self.command) == 6 and
            self.command[0] == '[' and
            self.command[1] in ['-x','-f','-e'] and
            self.command[2] == self.command[5] and
            self.command[3] == ']' and
            self.command[4] == '&&' ):
                self.testremoved = self.command[2]
                self.command = self.command[5:]

        if (len(self.command) == 5 and
            self.command[0] == 'test' and
            self.command[1] in ['-x','-f','-e'] and
            self.command[2] == self.command[4] and
            self.command[3] == '&&' ):
                self.testremoved = self.command[2]
                self.command = self.command[4:]
in the C++ rewrite as "brittle damage" ‒
https://github.com/systemd-cron/systemd-cron/pull/101#issuecomment-1673266966.

The only substitutions left in decode_command() that do explicit damage
to the line are
		if(auto pgm = this->which(this->command[0]); pgm && *pgm != this->command[0]) {
			if(!this->command.command0)
				++this->command.command.b;
			this->command.command0 = std::move(pgm);
		}
which dereferences the first word so we can avoid going through
/bin/sh copied-line.sh for single-word lines
(which, I've just realised, breaks when stuff lands in/is removed from,
 say /usr/local/bin, and systemd isn't reloaded ‒ imagine
   cp /bin/id /usr/local/bin
   systemd daemon-reload
   rm /usr/local/bin/id
 ‒ and now a "@daily id" job no longer works!
   since it tries executing /usr/local/bin/id!;
 IMO we should trim the whole function before
   if(!std::binary_search(std::begin(KSH_SHELLS), std::end(KSH_SHELLS), vore::basename(this->shell)))
 and axe KSH_SHELLS, and always generate a scriptlet)
and
		if(this->command.size() > 2 && this->command.command.e[-2] == ">"sv && this->command.command.e[-1] == "/dev/null"sv)
			this->command.command.e -= 2;
		if(this->command.size() > 1 && this->command.command.e[-1] == ">/dev/null"sv)
			this->command.command.e -= 1;
which you're seeing.

> We can either [1] remove the code that chop of the /dev/null
I agree. This had, maybe, potentially, some minute degree of merit
before we did normal cron-style mail.

Now that we behave like cron, it's just knowing-better-than-the-user.

> or [2] isolate it to only run when CRON_MAIL_SUCCESS=never
> (a setting that allow users to revert to the previous behaviour).
The user has all the controls to configure this precisely as they need now,
this is clearly confusing, since it goes against the explicit and overt
user configuration.

> I personally prefer "1" because of my own past habits
Agree, just kill it.

> even if "2" will look cleaner from a new point of view..
I don't think it is, really. Respect what the user wrote,
don't do needless magic (like this), and users will be happy.

Maybe add a changelog note that
"Fix: sd-cron-g no longer damages the line by stripping >[ ]/dev/null".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/pkg-systemd-maintainers/attachments/20230912/cb1171a6/attachment.sig>