Page MenuHomeFreeBSD
Differential D3254 Diff 7535 head/usr.bin/wall/ttymsg.c

Changeset View

Standalone View

View Options

head/usr.bin/wall/ttymsg.c

Show First 20 Lines • Show All 56 Lines • ▼ Show 20 Lines
* ignored (exclusive-use, lack of permission, etc.). * ignored (exclusive-use, lack of permission, etc.).
*/ */
const char * const char *
ttymsg(struct iovec *iov, int iovcnt, const char *line, int tmout) ttymsg(struct iovec *iov, int iovcnt, const char *line, int tmout)
{ {
struct iovec localiov[7]; struct iovec localiov[7];
ssize_t left, wret; ssize_t left, wret;
int cnt, fd; int cnt, fd;
static char device[MAXNAMLEN] = _PATH_DEV; char device[MAXNAMLEN] = _PATH_DEV;
pfgUnsubmitted
Not Done
Inline Actions

From bde@:

This removes the rather silly micro-optimization of using a static
buffer with just the few characters in _PATH_DEV in it held constant.
The buffer is now allocated locally and initialized by copying _PATH_DEV
to it on every call. If you want this pessimization, don't keep the
obfuscation of initializing it in its declaration. Use strcpy() or
snprintf() to initialize.

pfg: From bde@: This removes the rather silly micro-optimization of using a static buffer with just…
jmgUnsubmitted
Not Done
Inline Actions

either of on this one.. we should keep the micro op of not having it static IMO

jmg: either of on this one.. we should keep the micro op of not having it static IMO
opAuthorUnsubmitted
Not Done
Inline Actions

bde@:

Is this optimization really necessary in 2015 in a not performance sensitive code-path?

/*
 * Display the contents of a uio structure on a terminal.  Used by wall(1),
 * syslogd(8), and talkd(8).  Forks and finishes in child if write would block,
 * waiting up to tmout seconds.  Returns pointer to error string on unexpected
 * error; string is not newline-terminated.  Various "normal" errors are
 * ignored (exclusive-use, lack of permission, etc.).
 */
const char *
ttymsg(struct iovec *iov, int iovcnt, const char *line, int tmout)

This function are always called with 1 or 10 seconds! of timeout:

root@opbsd-fortify:/usr/src/usr.sbin/syslogd # grep -A1 ttymsg *
Makefile:SRCS=  syslogd.c ttymsg.c
Makefile-
Binary file syslogd matches
--
syslogd.c:#define       TTYMSGTIME      1               /* timeout passed to ttymsg */
syslogd.c-#define       RCVBUF_MINSIZE  (80 * 1024)     /* minimum size of dgram rcv buffer */
--
syslogd.c:#include "ttymsg.h"
syslogd.c-
--
syslogd.c:              errno = 0;      /* ttymsg() only sometimes returns an errno */
syslogd.c:              if ((msgret = ttymsg(iov, IOV_SIZE, f->f_un.f_fname, 10))) {
syslogd.c-                      f->f_type = F_UNUSED;
--
syslogd.c:                      if ((p = ttymsg(iov, iovlen, ut->ut_line,
syslogd.c-                          TTYMSGTIME)) != NULL) {
--
syslogd.c:                              if ((p = ttymsg(iov, iovlen, ut->ut_line,
syslogd.c-                                  TTYMSGTIME)) != NULL) {
op: bde@: Is this optimization really necessary in 2015 in a not performance sensitive code-path?
pfgUnsubmitted
Not Done
Inline Actions

bde sadly doesn't cuse code review software.

bde called it "silly optimization".
I agree with jmg@ and op@ that we don't need it.

pfg: bde sadly doesn't cuse code review software. bde called it "silly optimization". I agree with…
static char errbuf[1024]; static char errbuf[1024];
pfgUnsubmitted
Not Done
Inline Actions

From bde@:

Another static buffer. The function is obviously not reentrant. This
large static buffer mainly wastes space all the time instead of only
when the function is called.

pfg: From bde@: Another static buffer. The function is obviously not reentrant. This large static…
jmgUnsubmitted
Not Done
Inline Actions

but we don't know what errbuf is used for, a larger change is to make errbuf allocated, and require the caller to free it, but that is likely more invasive, but better change.

jmg: but we don't know what errbuf is used for, a larger change is to make errbuf allocated, and…
pfgUnsubmitted
Not Done
Inline Actions

Unstatic'ing this breaks the build.

pfg: Unstatic'ing this breaks the build.
char *p; char *p;
int forked; int forked;
forked = 0; forked = 0;
if (iovcnt > (int)(sizeof(localiov) / sizeof(localiov[0]))) if (iovcnt > (int)(sizeof(localiov) / sizeof(localiov[0])))
return ("too many iov's (change code in wall/ttymsg.c)"); return ("too many iov's (change code in wall/ttymsg.c)");
strlcat(device, line, sizeof(device));
pfgUnsubmitted
Not Done
Inline Actions

From bde@:

This depends on the slow reinitialization on every entry. The combined
initialization (initializer + strlcat) is an obfuscated way of
concatenating 2 strings. The clearest way is snprintf with %s%s format.

style(9) actually specifies using snprintf() indirectly. It never
dreamt of snprintf(), but it says to use printf() and not build up
output and bugs using fputs(), puts(), putchar(), whatever.

pfg: From bde@: This depends on the slow reinitialization on every entry. The combined…
jmgUnsubmitted
Not Done
Inline Actions

I managed to miss this on my first read through, and this is a good option..

jmg: I managed to miss this on my first read through, and this is a good option..
p = device + sizeof(_PATH_DEV) - 1; p = device + sizeof(_PATH_DEV) - 1;
strlcpy(p, line, sizeof(device));
pfgUnsubmitted
Not Done
Inline Actions

From bde:

The correct fix was just to use the right buffer size here. The pointer was adjusted to skip _PATH_BUF at the beginning of the buffer, but the buffer size was not adjusted to match.

pfg: From bde: The correct fix was just to use the right buffer size here. The pointer was…
if (strncmp(p, "pts/", 4) == 0) if (strncmp(p, "pts/", 4) == 0)
p += 4; p += 4;
if (strchr(p, '/') != NULL) { if (strchr(p, '/') != NULL) {
/* A slash is an attempt to break security... */ /* A slash is an attempt to break security... */
(void) snprintf(errbuf, sizeof(errbuf), (void) snprintf(errbuf, sizeof(errbuf),
"Too many '/' in \"%s\"", device); "Too many '/' in \"%s\"", device);
return (errbuf); return (errbuf);
} }
▲ Show 20 Lines • Show All 84 Lines • Show Last 20 Lines