Bug 112379 - [patch] [request] lockf(1): on closing stdin, stdout, stderr
Summary: [patch] [request] lockf(1): on closing stdin, stdout, stderr
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Kyle Evans
URL: https://reviews.freebsd.org/D42713
Keywords: patch
Depends on:
Blocks:
 
Reported: 2007-05-03 06:50 UTC by Alexander Melkov
Modified: 2023-12-15 18:59 UTC (History)
2 users (show)

See Also:
kevans: mfc-stable14+
kevans: mfc-stable13+
kevans: mfc-stable12-


Attachments
file.diff (486 bytes, patch)
2007-05-03 06:50 UTC, Alexander Melkov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Melkov 2007-05-03 06:50:04 UTC
Because lockf command forks but doesn't close input and output file handles 
in the main process, closing stdin, stdout or stderr inside child processes 
doesn't cause respective ends of pipes seen as closed by programs that read 
write to them.

For example:
(script1.sh | lockf foo script2.sh 2>&1 &) | mail -s something somewhere

Suppose that script2.sh closes its standard input and continues some long-time 
processing before script1.sh completes writing to its stdndard output.
script1.sh will hang for a long time (it would not without lockf).

As a more practical example, script2.sh redirects its output at some point
and continues its business. Because of lockf, mail will needlessly wait until
script2.sh completes.

I think that stdin should always be closed, and -s switch may be used as a flag
that closing stdout and stderr is desirable.

Fix: This is a patch against src/usr.bin/lockf/lockf.c,v 1.11.8.1
Comment 1 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 07:58:59 UTC
For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped
Comment 2 Graham Perrin freebsd_committer freebsd_triage 2022-10-17 12:34:52 UTC
Keyword: 

    patch
or  patch-ready

– in lieu of summary line prefix: 

    [patch]

* bulk change for the keyword
* summary lines may be edited manually (not in bulk). 

Keyword descriptions and search interface: 

    <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>
Comment 3 Kyle Evans freebsd_committer freebsd_triage 2023-11-22 05:51:03 UTC
Doing a round of work on lockf, I don't see any reason not to incorporate this young 16 year old patch.

I put it up for review @ https://reviews.freebsd.org/D42713, but I'm dropping the -s switching behavior because I just don't see any reason to keep them open at that point.
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-11-26 04:11:05 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=18425c19cae08cbe41801845457ed67285806688

commit 18425c19cae08cbe41801845457ed67285806688
Author:     Alexander Melkov <melkov@comptek.ru>
AuthorDate: 2023-11-22 04:46:28 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2023-11-26 04:09:27 +0000

    lockf: don't hold stdin/stdout/stderr open

    None of these are essential in the lockf monitor (parent post-fork), so
    close them to maintain the illusion that lockf hasn't been inserted into
    the pipeline.  This ensures that the correct effects happen on other
    programs in the pipeline if the locked command closes or redirects these
    elsewhere.

    The original patch used -s to close stdout/stderr rather than closing
    them unconditionally, but it's not clear that we really care that much.
    kevans dropped that part when taking the patch, patch is otherwise by
    listed author.

    PR:             112379
    Reviewed by:    0mp, allanjude (both earlier version), kevans
    Feedback from:  des
    Sponsored by:   Klara, Inc.
    Differential Revision:  https://reviews.freebsd.org/D42713

 usr.bin/lockf/lockf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2023-12-15 16:51:31 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=579c24898b89b87dc86095c3e55c1b1f8fca5e1e

commit 579c24898b89b87dc86095c3e55c1b1f8fca5e1e
Author:     Alexander Melkov <melkov@comptek.ru>
AuthorDate: 2023-11-22 04:46:28 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2023-12-15 00:58:20 +0000

    lockf: don't hold stdin/stdout/stderr open

    None of these are essential in the lockf monitor (parent post-fork), so
    close them to maintain the illusion that lockf hasn't been inserted into
    the pipeline.  This ensures that the correct effects happen on other
    programs in the pipeline if the locked command closes or redirects these
    elsewhere.

    The original patch used -s to close stdout/stderr rather than closing
    them unconditionally, but it's not clear that we really care that much.
    kevans dropped that part when taking the patch, patch is otherwise by
    listed author.

    PR:             112379
    Reviewed by:    0mp, allanjude (both earlier version), kevans
    Feedback from:  des
    Sponsored by:   Klara, Inc.

    (cherry picked from commit 18425c19cae08cbe41801845457ed67285806688)

 usr.bin/lockf/lockf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2023-12-15 18:57:49 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=159d922f4ce9405189b4f92bfe46858bfde75400

commit 159d922f4ce9405189b4f92bfe46858bfde75400
Author:     Alexander Melkov <melkov@comptek.ru>
AuthorDate: 2023-11-22 04:46:28 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2023-12-15 00:59:18 +0000

    lockf: don't hold stdin/stdout/stderr open

    None of these are essential in the lockf monitor (parent post-fork), so
    close them to maintain the illusion that lockf hasn't been inserted into
    the pipeline.  This ensures that the correct effects happen on other
    programs in the pipeline if the locked command closes or redirects these
    elsewhere.

    The original patch used -s to close stdout/stderr rather than closing
    them unconditionally, but it's not clear that we really care that much.
    kevans dropped that part when taking the patch, patch is otherwise by
    listed author.

    PR:             112379
    Reviewed by:    0mp, allanjude (both earlier version), kevans
    Feedback from:  des
    Sponsored by:   Klara, Inc.

    (cherry picked from commit 18425c19cae08cbe41801845457ed67285806688)

 usr.bin/lockf/lockf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Comment 7 Kyle Evans freebsd_committer freebsd_triage 2023-12-15 18:59:17 UTC
Thanks for the patch!