Bug 250691 - mail/fetchmail: Bad -c option
Summary: mail/fetchmail: Bad -c option
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Matthias Andree
URL:
Keywords:
Depends on: 250925
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-28 11:36 UTC by Helmut Ritter
Modified: 2020-11-08 11:19 UTC (History)
4 users (show)

See Also:
chalpin: maintainer-feedback+


Attachments
truss.log (74.29 KB, text/plain)
2020-10-29 15:28 UTC, Helmut Ritter
no flags Details
1st attempt at fixing the script (546 bytes, patch)
2020-10-31 23:29 UTC, Matthias Andree
chalpin: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Helmut Ritter 2020-10-28 11:36:49 UTC
Hi,

since the latest upgrade fetchmail does not start anymore:

[helmut@BSDHelmut ~]$ sudo /usr/local/etc/rc.d/fetchmail status
Bad -c option
Bad -c option
fetchmail is not running.
[helmut@BSDHelmut ~]$ sudo grep '\-c' /usr/local/etc/rc.d/fetchmail
        su -m ${fetchmail_user} -c sh -c "fetchmail -f ${fetchmail_config} --configdump" | fgrep $1 | cut -d: -f2
[helmut@BSDHelmut ~]$ uname -a
FreeBSD BSDHelmut.charlieroot.de 12.1-RELEASE-p8 FreeBSD 12.1-RELEASE-p8 r364293 GENERIC-PF-ALTQ  amd64
[helmut@BSDHelmut ~]$ pkg info -ix fetchmail
fetchmail-6.4.12_2
[helmut@BSDHelmut ~]$
Comment 1 Corey Halpin 2020-10-28 12:25:23 UTC
Can you please provide the rc.conf{.local} settings relevant to fetchmail?

Do you have any make.conf or src.conf settings that would have changed the build of either `su` or `sh` ?
Comment 2 Helmut Ritter 2020-10-28 12:50:21 UTC
(In reply to Corey Halpin from comment #1)

[helmut@BSDHelmut ~]$ sudo grep -ir fetchmail /etc/rc*
/etc/rc.conf:fetchmail_enable="YES"
/etc/rc.conf:fetchmail_polling_interval="900"
/etc/rc.conf:fetchmail_logging_facility="--syslog"
[helmut@BSDHelmut ~]$

I do not have anything in make.conf or src.conf related to su or sh.

I have a second machine just to build ports where it runs fine.

[helmut@BSDHelmut ~]$ sudo su -m fetchmail -c sh -c "echo test"
Bad -c option
[helmut@BSDHelmut ~]$

[helmut@BSDHelmut12 ~]$ sudo su -m fetchmail -c sh -c "echo test"
test
[helmut@BSDHelmut12 ~]$

[helmut@BSDHelmut ~]$ sudo su -m fetchmail -c sh -c "echo test"
Bad -c option
[helmut@BSDHelmut ~]$ sudo su -m fetchmail -c "echo test"
test
[helmut@BSDHelmut ~]$

Issue with the installation?!
Comment 3 Corey Halpin 2020-10-28 13:44:22 UTC
On the affected machine, what is root's login shell?

Are there entries in the affected machine's /etc/login.conf that would apply to the `sh` login class?

For what it's worth, 'Bad -c option' does not come from `su`, but from `sh` when the given command is empty. In 12.1, it's coming from `bin/sh/options.c` line 199: https://svnweb.freebsd.org/base/release/12.1.0/bin/sh/options.c?view=markup#l199 .  It's what one would get when running `sh -c` without actually providing any argument to the -c option.

What I can't (yet) figure out is how that could happen given the information you've provided.
Comment 4 Helmut Ritter 2020-10-28 14:11:28 UTC
(In reply to Corey Halpin from comment #3)

# id -u
0
# echo $SHELL
/bin/sh
#

login.conf is identical on both machines, stock on both machines. Contains nothing related to sh.

[helmut@BSDHelmut ~]$ sha256 /bin/sh
SHA256 (/bin/sh) = 4d8accb3e9c29c2901a5aefec3140baf13650a621ee1de0e9569d82a10b846c5
[helmut@BSDHelmut ~]$

[helmut@BSDHelmut12 ~]$ sha256 /bin/sh
SHA256 (/bin/sh) = 4d8accb3e9c29c2901a5aefec3140baf13650a621ee1de0e9569d82a10b846c5
[helmut@BSDHelmut12 ~]$
Comment 5 Corey Halpin 2020-10-29 14:30:29 UTC
Can you run `sudo truss -aef -o truss.log su -m fetchmail -c sh -c "echo test"` on the affected machine and either attach the resulting `truss.log`?
Comment 6 Corey Halpin 2020-10-29 14:31:05 UTC
(In reply to Corey Halpin from comment #5)

s/either//
Comment 7 Helmut Ritter 2020-10-29 15:28:14 UTC
Created attachment 219203 [details]
truss.log

sudo truss -aef -o truss.log su -m fetchmail -c sh -c "echo test"
Comment 8 Matthias Andree freebsd_committer freebsd_triage 2020-10-31 23:29:41 UTC
Created attachment 219266 [details]
1st attempt at fixing the script

Helmut, 

could you apply the patch in the files/ directory,
and then clean and reinstall fetchmail, and see if that fixes your problem?

Thanks.
Comment 9 Corey Halpin 2020-11-01 18:42:31 UTC
Comment on attachment 219266 [details]
1st attempt at fixing the script

Looks good to me, works in my testing. Thank you!
Comment 10 commit-hook freebsd_committer freebsd_triage 2020-11-01 19:39:29 UTC
A commit references this bug:

Author: mandree
Date: Sun Nov  1 19:38:59 UTC 2020
New revision: 553849
URL: https://svnweb.freebsd.org/changeset/ports/553849

Log:
  mail/fetchmail: Fix shell's 'Bad -c option' in rcscript.

  Turns out that our fetchmail_dump_config() function needs to add
  one more level of quoting because it's being unquoted and word split
  twice, once by su's shell, and again by sh.

  While here, change sh to /bin/sh to make the intention clearer.

  Bump PORTREVISION to get the fix out onto the systems.

  PR:		250691
  Reported by:	Helmut Ritter <freebsd-ports@charlieroot.de>
  Approved by:	chalpin@cs.wisc.edu
  MFH:		2020Q4 (blanket, one-line tested working fix, 4-eyes principle)

Changes:
  head/mail/fetchmail/Makefile
  head/mail/fetchmail/files/fetchmail.in
Comment 11 Matthias Andree freebsd_committer freebsd_triage 2020-11-01 19:40:26 UTC
Given Corey's verification of the fix, I have committed it, but Helmut, it would still be good to receive your OK, too.  You can also update to 6.4.12_3 for testing, which contains the fix.
Comment 12 commit-hook freebsd_committer freebsd_triage 2020-11-01 19:40:30 UTC
A commit references this bug:

Author: mandree
Date: Sun Nov  1 19:39:36 UTC 2020
New revision: 553850
URL: https://svnweb.freebsd.org/changeset/ports/553850

Log:
  MFH: r553849

  mail/fetchmail: Fix shell's 'Bad -c option' in rcscript.

  Turns out that our fetchmail_dump_config() function needs to add
  one more level of quoting because it's being unquoted and word split
  twice, once by su's shell, and again by sh.

  While here, change sh to /bin/sh to make the intention clearer.

  Bump PORTREVISION to get the fix out onto the systems.

  PR:		250691
  Reported by:	Helmut Ritter <freebsd-ports@charlieroot.de>
  Approved by:	chalpin@cs.wisc.edu

  Approved by:	ports-secteam@ (blanket, one-line tested working fix, 4-eyes principle)

Changes:
_U  branches/2020Q4/
  branches/2020Q4/mail/fetchmail/Makefile
  branches/2020Q4/mail/fetchmail/files/fetchmail.in
Comment 13 Helmut Ritter 2020-11-02 10:33:45 UTC
(In reply to Matthias Andree from comment #11)

Works for me, thank you!
Comment 14 Brian Biskeborn 2020-11-05 03:22:21 UTC
The inner shell in fetchmail_dump_config doesn't have its PATH set properly and can't execute "fetchmail", at least for me. Specifying the full path to the binary fixes it and is good practice anyway:

su -m ${fetchmail_user} -c "/bin/sh -c '/usr/local/bin/fetchmail -f ${fetchmail_config} --configdump'" | fgrep $1 | cut -d: -f2
Comment 15 Andrey Kiryanov 2020-11-05 17:07:58 UTC
(In reply to Brian Biskeborn from comment #14)
Same for me, the latest port release is still shipped with a broken rc script.

The only suggestion I have is to avoid extra shell invocation altogether. Here's my way of fixing things:

fetchmail_dump_config()
{
        su -m ${fetchmail_user} -c "${command} -f ${fetchmail_config} --configdump" | fgrep $1 | cut -d: -f2
}
Comment 16 Corey Halpin 2020-11-05 17:28:13 UTC
Brian and Andrey, could you specify in more detail how precisely you're running the script such that $PATH is not set correctly?

As for an 'extra shell invocation', this invocation quite intentional in order to get Bourne shell semantics and follows the usage of `su` from `rc.subr`. Rather than removing it, I'd suggest:

su -m ${fetchmail_user} -c "/bin/sh -c '${command} -f ${fetchmail_config} --configdump'" | fgrep $1 | cut -d: -f2

Which should work as long as /usr/bin is in $PATH. If /usr/bin weren't in path you would also be having trouble with `su`, so it seems like the above should work. But it would help to know how to replicate the issue to test and verify.
Comment 17 Brian Biskeborn 2020-11-05 17:42:19 UTC
I'm running fetchmail inside a 12.2-RELEASE jail. No special settings, I've simply enabled it in rc.conf.

The startup script runs with the default PATH set in /etc/rc, namely:
/sbin:/bin:/usr/sbin:/usr/bin

This doesn't include /usr/local/bin, where the fetchmail binary is installed.
Comment 18 Brian Biskeborn 2020-11-05 17:44:25 UTC
(In reply to Corey Halpin from comment #16)

I can reproduce this with a simple "service fetchmail start" invocation as well.
Comment 19 Corey Halpin 2020-11-05 17:55:12 UTC
(In reply to Brian Biskeborn from comment #18)

Logging in as root and running `service fetchmail start` is one of the ways this was tested before it was committed. With a default shell configuration, I do not see the problem you report when testing it that way.

I can reproduce when run from `/etc/rc` after a reboot, but the fix I suggested resolves it for me.
Comment 20 Andrey Kiryanov 2020-11-05 19:09:30 UTC
(In reply to Corey Halpin from comment #19)
I see exactly the same thing Brian does: invoking `service fetchmail {start|stop|status}' shows the following:

/bin/sh: fetchmail: not found
/bin/sh: fetchmail: not found

If you dump the environment from inside rc script you'll see that it's stripped to the bones:

PATH=/sbin:/bin:/usr/sbin:/usr/bin
PWD=/
_=/bin/sh
HOME=/
RC_PID=30789
SHLVL=0

Indeed, /usr/local/bin is not in the PATH.
Comment 21 Andrey Kiryanov 2020-11-05 19:16:36 UTC
(In reply to Corey Halpin from comment #16)
As for rc.subr su usage semantics, take a look at /etc/rc.d/ppp:

su -m $ppp_user -c "$command ${rc_flags} ${_ppp_profile}"

This is included in base system, so I still believe double shell invocation is pretty optional. The script itself has /bin/sh shebang so no need to worry about proper shell.
Comment 22 Corey Halpin 2020-11-05 20:23:12 UTC
(In reply to Andrey Kiryanov from comment #20)

That's true, but the environment inside the rc script is not the one that matters since `su` spawns its own shell that will have its own environment. Stripping down the environment where `su` runs doesn't strip the one that `su` creates.

On my system, with default shell and default shell configuration:

# env PATH='/sbin:/bin:/usr/sbin:/usr/bin' su -m root -c "/bin/sh -c 'echo \$PATH'"
/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin:/root/bin

# env PATH='/sbin:/bin:/usr/sbin:/usr/bin' su -m root -c 'echo $PATH'
/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin:/root/bin
Comment 23 Corey Halpin 2020-11-05 20:49:32 UTC
Oh. I'm blaming `su` when it's really `csh` I should be pointing at.

$ env PATH='/sbin:/bin:/usr/sbin:/usr/bin' /bin/csh -c "echo \$PATH"  
/sbin:/bin:/usr/sbin:/usr/bin:/usr/games:/usr/local/sbin:/usr/local/bin:/home/crhalpin/bin

compared with

$ env PATH='/sbin:/bin:/usr/sbin:/usr/bin' /bin/sh -c "echo \$PATH"  
/sbin:/bin:/usr/sbin:/usr/bin

Or even

$ env PATH='/sbin:/bin:/usr/sbin:/usr/bin' /bin/csh -f -c "echo \$PATH"  
/sbin:/bin:/usr/sbin:/usr/bin
Comment 24 Andrey Kiryanov 2020-11-06 15:07:02 UTC
(In reply to Corey Halpin from comment #23)

So, well, while rc script's environment is pretty well defined, further nested shell invocation can differ substantially between systems. That's why, as Brian mentioned earlier, it makes perfect sense to always use fully qualified path names.
Comment 25 Corey Halpin 2020-11-06 15:26:34 UTC
(In reply to Andrey Kiryanov from comment #24)

Yes, I agree, hence the suggestion of `
su -m ${fetchmail_user} -c "/bin/sh -c '${command} -f ${fetchmail_config} --configdump'" | fgrep $1 | cut -d: -f2` that I made in comment 16.


The point of my comments 22 and 23 was twofold.

The first point of these comments was to document that, contrary to comments 18 and 20 a simple `service fetchmail start` is not sufficient to reproduce the problem with a default configuration. The version of the script from the package *does* work with a default configuration, thought it works more by accident than by design. Clearly not an ideal situation and clearly something that should be remedied.

The second point of these comments was to document the accident that allowed the script to function in my testing. I wanted this explicitly written down so that a) I can extend my testing to cover this situation in the future, b) when I come back to this bug in the future it will be clear what happened, and c) any other port maintainer who happens on a similar issue and finds this bug will also be able to understand what went wrong.
Comment 26 Corey Halpin 2020-11-06 15:52:35 UTC
(and when I say "default configuration", I mean one where root's shell is `csh` and `/root/.cshrc` contains https://svnweb.freebsd.org/base/release/12.2.0/bin/csh/dot.cshrc?view=markup , as it does on all the systems and jails where I've been testing )
Comment 27 Andrey Kiryanov 2020-11-06 20:40:11 UTC
(In reply to Corey Halpin from comment #26)
I agree with you on both points. As we can see from real-life examples, "default configuration" is not something you encounter on every system out there. That's why we need some extra testing and caution with our assumptions when we design something that's gotta work out-of-the-box on most systems.
Comment 28 Matthias Andree freebsd_committer freebsd_triage 2020-11-06 21:07:00 UTC
Well, let's reopen this for the nonce.

Corey, do you want me to commit your proposal from comment #16, or is 6.4.13 so close in time that we don't need to bother?
Comment 29 Corey Halpin 2020-11-06 21:18:49 UTC
(In reply to Matthias Andree from comment #28)

I've got my regular battery of test fetchmail configurations doing `poudriere testport` on 6.4.13 (plus comment 16) right now.

Barring any problems, I expect to have a patch ready later tonight or early in the morning (for values of 'tonight' and 'morning' consistent with UTC-0600).
Comment 30 Corey Halpin 2020-11-08 01:33:30 UTC
(In reply to Corey Halpin from comment #29)

FYI: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=250925
Comment 31 commit-hook freebsd_committer freebsd_triage 2020-11-08 10:59:00 UTC
A commit references this bug:

Author: mandree
Date: Sun Nov  8 10:58:43 UTC 2020
New revision: 554447
URL: https://svnweb.freebsd.org/changeset/ports/554447

Log:
  mail/fetchmail: mail/fetchmailconf: Update to 6.4.13 [1], fix rcfile bug [2]

  Update mail/fetchmail{,conf} to 6.4.13 and fix rc script to work correctly
  when root's shell does not include /usr/local/bin in $PATH.

  mail/fetchmail passes 'poudriere testport' on both i386 and amd64 under
  11.4 and 12.1 for the following configurations:
    - Default settings
    - Default settings, build as non-root
    - ssl=base, GSSAPI_MIT
    - ssl=base, GSSAPI_NONE
    - ssl=openssl
    - ssl=openssl with SSL2 and SSL3 disabled
    - ssl=openssl, GSSAPI_NONE
    - ssl=libressl
    - ssl=libressl, GSSAPI_NONE

  mail/fetchmailconf passes 'poudriere testport' on both i386 and amd64 under
  11.4 and 12.1 with default settings

  Additionally, passes bulk -tC on 12.1-arm64.

  PR:		250925 [1]
  Submitted by:	Corey Halpin (maintainer)
  PR:		250691 [2, comments #14, #15]
  Reported by:	Brian Biskeborn [2], Andrey Kiryanov [2]

Changes:
  head/mail/fetchmail/Makefile
  head/mail/fetchmail/distinfo
  head/mail/fetchmail/files/fetchmail.in
Comment 32 Matthias Andree freebsd_committer freebsd_triage 2020-11-08 10:59:28 UTC
Fixed with the port update to fetchmail 6.4.13.
Comment 33 commit-hook freebsd_committer freebsd_triage 2020-11-08 11:19:08 UTC
A commit references this bug:

Author: mandree
Date: Sun Nov  8 11:18:18 UTC 2020
New revision: 554449
URL: https://svnweb.freebsd.org/changeset/ports/554449

Log:
  Fix rc script when root's shell does not include /usr/local/bin in $PATH

  This is the blanket-approved safe part of PR 250925,
  as partial MFH r554447.

  Let's give the pidfile changes in 6.4.13 some time to mature before
  MFH'ing them.

  PR:		250691
  PR:		250925 (partial MFH)
  Submitted by:	Corey Halpin (maintainer)
  Approved by:	ports-secteam@ (blanket for single-line tested fixes)

Changes:
  branches/2020Q4/mail/fetchmail/Makefile
  branches/2020Q4/mail/fetchmail/files/fetchmail.in