Bug 274454 - usr.sbin/syslogd: closing a configured terminal in parse_action(), line 3057
Summary: usr.sbin/syslogd: closing a configured terminal in parse_action(), line 3057
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 15.0-CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Jake Freeland
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-13 17:48 UTC by Trond Endrestøl
Modified: 2023-10-16 13:13 UTC (History)
3 users (show)

See Also:


Attachments
Patch for usr.sbin/syslogd/syslogd.c disabling closing of a terminal. (625 bytes, patch)
2023-10-13 17:48 UTC, Trond Endrestøl
no flags Details | Diff
Patch for usr.sbin/syslogd/syslogd.c disabling closing of a terminal. (727 bytes, patch)
2023-10-13 19:34 UTC, Trond Endrestøl
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Trond Endrestøl 2023-10-13 17:48:07 UTC
Created attachment 245605 [details]
Patch for usr.sbin/syslogd/syslogd.c disabling closing of a terminal.

Since forever I've run syslogd(8) configured to show all log lines on /dev/ttyvb, i.e.

*.*;mark.* /dev/ttyvb

Recent changes in CURRENT made /dev/ttyvb inactive. I've tracked the issue down to closing the terminal in parse_action() in usr.sbin/syslogd/syslogd.c on line 3057.

I therefore suggest to omit closing a terminal in case it's a VTY. Without such a change, a VTY is inaccessible by the user and completely useless.

Feel free to rework or reject the attached patch.
Comment 1 Trond Endrestøl 2023-10-13 19:34:05 UTC
Created attachment 245606 [details]
Patch for usr.sbin/syslogd/syslogd.c disabling closing of a terminal.

Better not lose the file descriptor.
Comment 2 Trond Endrestøl 2023-10-14 06:26:28 UTC
(In reply to Trond Endrestøl from comment #0)
The “offending” change is part of c3175a6e1c5a4b6337368ea140e3a3837b327ef2.
https://cgit.freebsd.org/src/commit/usr.sbin/syslogd/syslogd.c?id=c3175a6e1c5a4b6337368ea140e3a3837b327ef2
Comment 3 Jake Freeland 2023-10-14 16:47:00 UTC
(In reply to Trond Endrestøl from comment #0)
Hi Trond,

I'm not really sure what you mean by "made /dev/ttyvb inactive".

A TTY descriptor is only opened in parse_action() to check whether it is a valid tty using isatty(). That descriptor is closed because it will never be accessed again.

See https://cgit.freebsd.org/src/tree/usr.sbin/syslogd/syslogd.c?id=a5ed6a815e38d6c622cd97a6020592ded579cf7a#n1973

When a log message is sent to a TTY, its path (saved in f->fu_fname) is passed into ttymsg() and used to reopen the TTY under a new, temporary descriptor. That original descriptor value (f->f_file) is never actually used.

Did your patch fix the issue you were facing? If so, there is something I am not seeing here.
Comment 4 Trond Endrestøl 2023-10-15 17:15:17 UTC
(In reply to Jake Freeland from comment #3)
> Did your patch fix the issue you were facing? If so, there is something I am not seeing here.

Yes, my change made ttyvb selectable using Alt+F12. Without this change I can select Alt+F12 until I'm blue in my face and never see the contents of that VTY.

ttymsg() might be doing its thing, but it closes the terminal after writing the log message, line 162 of usr.bin/wall/ttymsg.c. If no process holds ttyvb open, my claim is that the VTY isn't accessible to the user.
Comment 5 Trond Endrestøl 2023-10-15 17:24:59 UTC
(In reply to Trond Endrestøl from comment #4)
Actually, line 154 of usr.bin/wall/ttymsg.c is more likely to close the file descriptor opened on line 88.
Comment 6 Jake Freeland 2023-10-15 21:52:38 UTC
(In reply to Trond Endrestøl from comment #4)
I just submitted a patch for review that addresses this issue. Let me know if everything looks okay.

https://reviews.freebsd.org/D42215
Comment 7 Trond Endrestøl 2023-10-16 07:09:46 UTC
I created a simple experiment:

1. Install FreeBSD 15.0-CURRENT from the latest snapshot, i.e. 20231005-8818f0f1124e-265728.
2. Verify nothing happens when you hit Alt+F12.
3. echo '*.*;mark.* /dev/ttyvb' > /etc/syslogd.d/local.conf
4. Verify that syslogd is running, noting its PID, service syslogd status.
5. Restart syslogd, service syslogd restart.
4. Verify that syslogd is still running, noting its PID, which should now be different, service syslogd status.
6. Attempt to switch to /dev/ttyvb.
7. Note nothing happens.
8. Try steps 2-7 on any older versions of FreeBSD, maybe adjusting step 3 on systems lacking the syslogd.d functionality.
9. Older systems exhibit a different behaviour than recent 15.0-CURRENT.
Comment 8 Trond Endrestøl 2023-10-16 07:34:01 UTC
(In reply to Jake Freeland from comment #6)
Patch looks good to me. It is in effect identical to my own change.
Comment 9 commit-hook freebsd_committer freebsd_triage 2023-10-16 13:12:49 UTC
A commit in branch main references this bug:

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

commit d556719e15d643ba9619bbbeab550eed87614525
Author:     Jake Freeland <jfree@FreeBSD.org>
AuthorDate: 2023-10-15 20:34:06 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-10-16 13:11:58 +0000

    syslogd: Keep console and tty descriptors open

    Console and tty descriptors are initially opened in parse_action() to
    determine whether they are valid using isatty(). That descriptor is then
    closed because it is never accessed by syslogd again; ttymsg() will reopen
    the tty/console under a new descriptor when needed.

    If the user attempts to log to a tty that is inactive outside of
    syslogd, then syslogd must keep that descriptor open so the tty remains
    accessible. For example, logging to `/dev/ttyvb` requires the initial
    `/dev/ttyvb` descriptor to stay open so the user can view its buffer at
    any time via CTRL+ALT+F12.

    As a result, console and tty descriptors must remain open until a
    potential configuration reload or a system shutdown. The given
    descriptor will be closed in close_filed() in such circumstances.

    PR:             274454
    Fixes:          c3175a6e1c5a ("syslogd: Do not open console descriptor")
    Reported by:    Trond Endrestøl <Trond.Endrestol@ximalas.info>
    Reviewed by:    markj
    Differential Revision:  https://reviews.freebsd.org/D42215

 usr.sbin/syslogd/syslogd.c | 3 ---
 1 file changed, 3 deletions(-)
Comment 10 Mark Johnston freebsd_committer freebsd_triage 2023-10-16 13:13:13 UTC
Thank you for the report.