Bug 234885 - cmp(1) Capsicum error if stdin closed
Summary: cmp(1) Capsicum error if stdin closed
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.0-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-01-11 20:07 UTC by Christian Weisgerber
Modified: 2019-12-06 18:28 UTC (History)
4 users (show)

See Also:


Attachments
patch (1.76 KB, patch)
2019-01-14 03:47 UTC, Mark Johnston
no flags Details | Diff
patch (2.01 KB, patch)
2019-01-14 04:13 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Weisgerber freebsd_committer freebsd_triage 2019-01-11 20:07:05 UTC
cmp(1) errors out with a Capsicum error when called with stdin closed:

$ sh -c 'cmp /bin/ls /bin/cat <&-'
cmp: /bin/ls: Capabilities insufficient

Since fd 0 is closed, open() of the first file argument returns fd=0, and a subsequent fstat(0, ...) produces the error.


I found this because GNU tar 1.31's test suite includes some test scripts that, for whatever reason, close stdin (exec <&-) and later call cmp.
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2019-01-14 03:47:17 UTC
Created attachment 201110 [details]
patch

The attached patch should fix this.  We have the same problem if stdout or stderr
is closed, since cmp(1) calls caph_limit_{stdout,stderr}().  Also, if all three streams are closed, the caph_limit_stderr() call will fail because STDERR_FILENO isn't a valid fd.

These problems aren't specific to cmp(1), they'll affect anything using these helpers.  I think the fcntl() calls in the patch should be lifted into the corresponding Capsicum helpers.
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2019-01-14 04:13:22 UTC
Created attachment 201112 [details]
patch

Here's a somewhat nicer patch that converts cmp(1) to use caph_limit_stdio() instead, which ignores EBADF.
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2019-01-16 20:40:53 UTC
https://reviews.freebsd.org/D18860
Comment 4 commit-hook freebsd_committer freebsd_triage 2019-01-17 17:37:08 UTC
A commit references this bug:

Author: markj
Date: Thu Jan 17 17:36:18 UTC 2019
New revision: 343117
URL: https://svnweb.freebsd.org/changeset/base/343117

Log:
  Fix handling of rights on stdio streams.

  - Limit rights on stdio before opening input files.  Otherwise, open()
    may return one of the standard descriptors and we end up limiting
    rights such that we cannot read from one of the input files.
  - Use caph_limit_stdio(), which suppresses EBADF, to ensure that
    we don't emit an error if one of the stdio streams is closed.
  - Don't bother further limiting rights on stdin when stdin isn't going
    to be used.  Doing so correctly requires checking for a number of
    edge cases, and it doesn't provide any significant benefit.

  PR:		234885
  Reviewed by:	oshogbo
  MFC after:	3 days
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D18860

Changes:
  head/usr.bin/cmp/cmp.c
Comment 5 commit-hook freebsd_committer freebsd_triage 2019-01-20 00:59:30 UTC
A commit references this bug:

Author: markj
Date: Sun Jan 20 00:58:55 UTC 2019
New revision: 343205
URL: https://svnweb.freebsd.org/changeset/base/343205

Log:
  MFC r343117:
  Fix handling of rights on stdio streams.

  PR:	234885

Changes:
_U  stable/12/
  stable/12/usr.bin/cmp/cmp.c
Comment 6 commit-hook freebsd_committer freebsd_triage 2019-01-21 03:47:59 UTC
A commit references this bug:

Author: markj
Date: Mon Jan 21 03:47:21 UTC 2019
New revision: 343245
URL: https://svnweb.freebsd.org/changeset/base/343245

Log:
  Revert r343117.

  It breaks the special mode specified by passing "-" as one of the
  input files.  Revert for now while we discuss a fix.

  PR:		234885
  Reported by:	delphij
  MFC after:	now

Changes:
  head/usr.bin/cmp/cmp.c
Comment 7 commit-hook freebsd_committer freebsd_triage 2019-02-25 19:47:52 UTC
A commit references this bug:

Author: markj
Date: Mon Feb 25 19:47:28 UTC 2019
New revision: 344551
URL: https://svnweb.freebsd.org/changeset/base/344551

Log:
  Fix handling of rights on stdio streams, take two.

  Split the rights-limiting code into two cases: if one of the input
  files isn't a regular file, use caph_limit_stream(3) instead of
  open-coding the same logic; if both input files are regular files,
  and the initial attempts to map them succeed, we limit the rights on
  those files to CAP_MMAP_R.

  Add a regression test for PR 234885.

  PR:		234885
  Reviewed by:	delphij
  MFC after:	2 weeks
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D19216

Changes:
  head/usr.bin/cmp/cmp.c
  head/usr.bin/cmp/regular.c
  head/usr.bin/cmp/special.c
  head/usr.bin/cmp/tests/cmp_test2.sh
Comment 8 commit-hook freebsd_committer freebsd_triage 2019-12-06 18:28:21 UTC
A commit references this bug:

Author: markj
Date: Fri Dec  6 18:27:51 UTC 2019
New revision: 355456
URL: https://svnweb.freebsd.org/changeset/base/355456

Log:
  MFC r344551:
  Fix handling of rights on stdio streams, take two.

  PR:	234885

Changes:
_U  stable/12/
  stable/12/usr.bin/cmp/cmp.c
  stable/12/usr.bin/cmp/regular.c
  stable/12/usr.bin/cmp/special.c
  stable/12/usr.bin/cmp/tests/cmp_test2.sh