ttymsg: Overhaul

[des]

Summary

• Instead of an error string, return the usual 0 or -1 and let the caller figure out what, if anything, to tell the user.

• Avoid string manipulations by opening /dev first and using openat() with O_RESOLVE_BENEATH.

• Add a boolean argument which, if false, causes ttymsg() to return without sending the message if the tty's group-writable bit is not set. This saves programs that respect this setting (like syslogd(8)) from having to check before calling ttymsg().

• Update all callers.

The observable effect of this change is minimal except for slightly
different error messages when ttymsg() fails. However, syslogd(8) will
no longer print spurious error messages on the console after trying and
failing to write a log message to an X11 session.

PR: 295171
MFC after: 1 week

[mail_fbsd2.e4m.org]

Is it enough to check the first character? In other words: Is it impossible to have something in front
of the ':' (like "localhost" or whatever)? That's why I used strchr() in my bug report...

[markj]

usr.sbin/syslogd/syslogd.c

2152

Why not instead check whether /dev/ut->ut_line exists?

[des]

usr.sbin/syslogd/syslogd.c

2152

It's nontrivial and repeats work later done by ttymsg(). I think it's time the latter got a glow-up...

[des]

redo

[markj]

usr.bin/wall/ttymsg.c

84

This leaks fd if fstat() fails.

usr.sbin/syslogd/syslogd_cap_log.c

138

Shouldn't we nvlist_get_boolean(nvlin, "shout") here?

143

I think you're supposed to pack this error value into nvlout? Returning a non-zero value to the caller here raises an error to the casper library rather than to the caller.

[des]

usr.sbin/syslogd/syslogd_cap_log.c

138

Good catch, thank you.

[des]

rf

[markj]

usr.sbin/syslogd/syslogd.c

2152

Do you still need this check?

[des]

usr.sbin/syslogd/syslogd.c

2152

No, I'd just forgotten about it.

[des]

rf

[markj]

usr.sbin/syslogd/syslogd_cap_log.c

112

I think the nvlist_destroy() and logerror() calls below could clobber errno.

[des]

@oshogbo can you please review the changes to the Casper code?

[des]

errno

[oshogbo]

For you, always, des :)

usr.bin/wall/ttymsg.c

50–54

Small nite: I guess we can drop double spaces.

68–70

Nite: I guess you can use nitems here.

usr.sbin/syslogd/syslogd_cap_log.c

141

I think this is won't work.
Libcasper handles the error handling:
https://github.com/freebsd/freebsd-src/blob/a2f733abcff64628b7771a47089628b7327a88bd/lib/libcasper/libcasper/service.c#L319-L324

So creating a duplicate "error" entry will brake libnv communication. So I guess you should do:

if (ttymsg(iov, iovcnt, line, tmout, shout) != 0) {
  serror = errno;
  free(iov);
  return (serror);
}

[des]

usr.sbin/syslogd/syslogd_cap_log.c

141

That's what I suspected. In that case I think cap_p_open() / casper_p_open() is incorrect as well.

To be clear, error contains the return value from the casper_*() function if and only if it is not zero, right?

[des]

usr.sbin/syslogd/syslogd_cap_log.c

141

Furthermore, cap_p_open() / casper_p_open() probably belong somewhere else, maybe in cap_fileargs() or maybe in its own Casper service, but let's leave that for later.

[des]

usr.bin/wall/ttymsg.c

50–54

Double spaces are necessary to distinguish the period at the end of a sentence from the period at the end of an abbreviation within a sentence (this is also why *roff and mandoc require sentences to start on a new line).

[oshogbo]

usr.sbin/syslogd/syslogd_cap_log.c

141

To be clear, error contains the return value from the casper_*() function if and only if it is not zero, right?

The error is always added - when the function is called. If the function return 0 then it is 0.

[des]

casper

[des]

nvlout

[des]

usr.sbin/syslogd/syslogd_cap_log.c

87

This should return an error code, not -1, but I'm not sure which. ECAPMODE perhaps?

[jfree]

usr.bin/wall/ttymsg.c

57

Is there a reason why we don't just make iovcnt a size_t while we're here?

57

I think we should rename the line parameter to something like tty. I needed to go and look at callsites to understand was supposed to be passed here. I understand it means line in the context of a serial line, but that wasn't immediately clear to me.

80

You removed the comment explaining why we don't return error on EBUSY and EACCES. I don't like this behavior, but if we're going to keep it, keep the explanation as well.

136

close might clobber errno here

usr.bin/wall/wall.c

162

Since ttymsg preserves errno now, we can use warn instead of warnx

usr.sbin/syslogd/syslogd_cap_log.c

87

I don't know if I'd call this a capability mode violation (which is what ECAPMODE is used for). Maybe just ENOENT since a matching filed couldn't be found.

101–102

Is there a reason you changed this from tmout to timeout? The surrounding context already uses tmout

[jfree]

While we're here, maybe steal NetBSD's ttymsg man page and adapt it for these changes? https://man.netbsd.org/ttymsg.3

[des]

In D57018#1310136, @jfree wrote:

While we're here, maybe steal NetBSD's ttymsg man page and adapt it for these changes? https://man.netbsd.org/ttymsg.3

We don't provide ttymsg(3).

usr.bin/wall/ttymsg.c

57

Is there a reason why we don't just make iovcnt a size_t while we're here?

writev() takes an int.

usr.sbin/syslogd/syslogd_cap_log.c

87

Exacly, it's a capability mode violation because we're trying to popen() a program that's not on the preapproved list.

101–102

I find the abbreviation confusing, especially next to shout which is not an abbreviation but happens to differ only by its first two letters, and this is not a situation where brevity would be preferred over clarity.

[des]

rf