Page MenuHomeFreeBSD
Differential D16518 Diff 46364 usr.sbin/newsyslog/newsyslog.c

Changeset View

Standalone View

View Options

usr.sbin/newsyslog/newsyslog.c

Show First 20 Lines • Show All 54 Lines • ▼ Show 20 Lines
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#define OSF #define OSF
#include <sys/param.h> #include <sys/param.h>
#include <sys/queue.h> #include <sys/queue.h>
#include <sys/sbuf.h>
#include <sys/stat.h> #include <sys/stat.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <assert.h> #include <assert.h>
#include <ctype.h> #include <ctype.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <dirent.h> #include <dirent.h>
Show All 11 Lines
#include <syslog.h> #include <syslog.h>
#include <time.h> #include <time.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#include "extern.h" #include "extern.h"
/* /*
* Compression suffixes
*/
#ifndef COMPRESS_SUFFIX_GZ
#define COMPRESS_SUFFIX_GZ ".gz"
#endif
#ifndef COMPRESS_SUFFIX_BZ2
#define COMPRESS_SUFFIX_BZ2 ".bz2"
#endif
#ifndef COMPRESS_SUFFIX_XZ
#define COMPRESS_SUFFIX_XZ ".xz"
#endif
#ifndef COMPRESS_SUFFIX_ZST
#define COMPRESS_SUFFIX_ZST ".zst"
#endif
#define COMPRESS_SUFFIX_MAXLEN MAX(MAX(MAX(sizeof(COMPRESS_SUFFIX_GZ),sizeof(COMPRESS_SUFFIX_BZ2)),sizeof(COMPRESS_SUFFIX_XZ)),sizeof(COMPRESS_SUFFIX_ZST))
/*
* Compression types * Compression types
*/ */
#define COMPRESS_TYPES 5 /* Number of supported compression types */ #define COMPRESS_TYPES 5 /* Number of supported compression types */
#define COMPRESS_NONE 0 #define COMPRESS_NONE 0
#define COMPRESS_GZIP 1 #define COMPRESS_GZIP 1
#define COMPRESS_BZIP2 2 #define COMPRESS_BZIP2 2
#define COMPRESS_XZ 3 #define COMPRESS_XZ 3
Show All 28 Lines
#define DEFAULT_TIMEFNAME_FMT "%Y%m%dT%H%M%S" #define DEFAULT_TIMEFNAME_FMT "%Y%m%dT%H%M%S"
#define MAX_OLDLOGS 65536 /* Default maximum number of old logfiles */ #define MAX_OLDLOGS 65536 /* Default maximum number of old logfiles */
struct compress_types { struct compress_types {
const char *flag; /* Flag in configuration file */ const char *flag; /* Flag in configuration file */
const char *suffix; /* Compression suffix */ const char *suffix; /* Compression suffix */
const char *path; /* Path to compression program */ const char *path; /* Path to compression program */
char **args; /* Compression program arguments */ const char **flags; /* Compression program flags */
int nargs; /* Program argument count */ int nflags; /* Program flags count */
}; };
static char f_arg[] = "-f"; static const char *gzip_flags[] = { "-f" };
static char q_arg[] = "-q"; #define bzip2_flags gzip_flags
static char rm_arg[] = "--rm"; #define xz_flags gzip_flags
static char *gz_args[] = { NULL, f_arg, NULL, NULL }; static const char *zstd_flags[] = { "-q", "--rm" };
markjUnsubmitted
Done
Inline Actions

The addition of -f should be a separate change.

markj: The addition of -f should be a separate change.
#define bz2_args gz_args
#define xz_args gz_args
static char *zstd_args[] = { NULL, q_arg, rm_arg, NULL, NULL };
static const struct compress_types compress_type[COMPRESS_TYPES] = { static const struct compress_types compress_type[COMPRESS_TYPES] = {
{ "", "", "", NULL, 0}, { "", "", "", NULL, 0 },
{ "Z", COMPRESS_SUFFIX_GZ, _PATH_GZIP, gz_args, nitems(gz_args) }, { "Z", ".gz", _PATH_GZIP, gzip_flags, nitems(gzip_flags) },
{ "J", COMPRESS_SUFFIX_BZ2, _PATH_BZIP2, bz2_args, nitems(bz2_args) }, { "J", ".bz2", _PATH_BZIP2, bzip2_flags, nitems(bzip2_flags) },
{ "X", COMPRESS_SUFFIX_XZ, _PATH_XZ, xz_args, nitems(xz_args) }, { "X", ".xz", _PATH_XZ, xz_flags, nitems(xz_flags) },
{ "Y", COMPRESS_SUFFIX_ZST, _PATH_ZSTD, zstd_args, nitems(zstd_args) } { "Y", ".zst", _PATH_ZSTD, zstd_flags, nitems(zstd_flags) }
}; };
struct conf_entry { struct conf_entry {
STAILQ_ENTRY(conf_entry) cf_nextp; STAILQ_ENTRY(conf_entry) cf_nextp;
char *log; /* Name of the log */ char *log; /* Name of the log */
char *pid_cmd_file; /* PID or command file */ char *pid_cmd_file; /* PID or command file */
char *r_reason; /* The reason this file is being rotated */ char *r_reason; /* The reason this file is being rotated */
int firstcreate; /* Creating log for the first time (-C). */ int firstcreate; /* Creating log for the first time (-C). */
▲ Show 20 Lines • Show All 1,835 Lines • ▼ Show 20 Lines if (secs > 0) {
sleep(secs); sleep(secs);
} }
} }
} }
static void static void
do_zipwork(struct zipwork_entry *zwork) do_zipwork(struct zipwork_entry *zwork)
{ {
const char *pgm_name, *pgm_path; const struct compress_types *ct;
int errsav, fcount, zstatus; struct sbuf *command;
pid_t pidzip, wpid; pid_t pidzip, wpid;
char zresult[MAXPATHLEN]; int c, errsav, fcount, zstatus;
char command[BUFSIZ]; const char **args, *pgm_name, *pgm_path;
char **args; char *zresult;
int c, i, nargs;
command = NULL;
assert(zwork != NULL); assert(zwork != NULL);
pgm_path = NULL; assert(zwork->zw_conf != NULL);
markjUnsubmitted
Done
Inline Actions

I would make a const local pointer to compress_type[zwork->zw_conf->compress], and use that here. You could get rid of the "nargs" local variable in the process.

markj: I would make a const local pointer to compress_type[zwork->zw_conf->compress], and use that…
dnelson_1901_yahoo.comAuthorUnsubmitted
Done
Inline Actions

I actually did that in my initial draft to reduce line length so every line didn't wrap, but after I removed the nested loop there was enough space so I removed it :) Easy enough to put back.

dnelson_1901_yahoo.com: I actually did that in my initial draft to reduce line length so every line didn't wrap, but…
markjUnsubmitted
Done
Inline Actions

You might consider asserting that the index is valid.

markj: You might consider asserting that the index is valid.
strlcpy(zresult, zwork->zw_fname, sizeof(zresult)); assert(zwork->zw_conf->compress > COMPRESS_NONE);
if (zwork->zw_conf != NULL && assert(zwork->zw_conf->compress < COMPRESS_TYPES);
zwork->zw_conf->compress > COMPRESS_NONE)
for (c = 1; c < COMPRESS_TYPES; c++) {
if (zwork->zw_conf->compress == c) {
nargs = compress_type[c].nargs;
args = calloc(nargs, sizeof(*args));
if (args == NULL)
err(1, "calloc()");
pgm_path = compress_type[c].path;
(void) strlcat(zresult,
compress_type[c].suffix, sizeof(zresult));
/* the first argument is always NULL, skip it */
for (i = 1; i < nargs; i++) {
if (compress_type[c].args[i] == NULL)
break;
args[i] = compress_type[c].args[i];
}
break;
}
}
if (pgm_path == NULL) {
warnx("invalid entry for %s in do_zipwork", zwork->zw_fname);
return;
}
pgm_name = strrchr(pgm_path, '/');
if (pgm_name == NULL)
pgm_name = pgm_path;
else
pgm_name++;
args[0] = strdup(pgm_name);
if (args[0] == NULL)
err(1, "strdup()");
for (c = 0; args[c] != NULL; c++)
;
args[c] = zwork->zw_fname;
if (zwork->zw_swork != NULL && zwork->zw_swork->sw_runcmd == 0 && if (zwork->zw_swork != NULL && zwork->zw_swork->sw_runcmd == 0 &&
zwork->zw_swork->sw_pidok <= 0) { zwork->zw_swork->sw_pidok <= 0) {
warnx( warnx(
"log %s not compressed because daemon(s) not notified", "log %s not compressed because daemon(s) not notified",
zwork->zw_fname); zwork->zw_fname);
change_attrs(zwork->zw_fname, zwork->zw_conf); change_attrs(zwork->zw_fname, zwork->zw_conf);
return; return;
} }
strlcpy(command, pgm_path, sizeof(command)); ct = &compress_type[zwork->zw_conf->compress];
/*
* execv will be called with the array [ program, flags ... ,
* filename, NULL ] so allocate nflags+3 elements for the array.
*/
args = calloc(ct->nflags + 3, sizeof(*args));
if (args == NULL)
err(1, "calloc");
pgm_path = ct->path;
pgm_name = strrchr(pgm_path, '/');
if (pgm_name == NULL)
pgm_name = pgm_path;
else
pgm_name++;
/* Build the argument array. */
args[0] = pgm_name;
markjUnsubmitted
Done
Inline Actions

Since you're using __DECONST below, why not here too?

Actually, I think you can instead declare "args" to be of type const char ** and just have one __DECONST in the execv() call:

execv(pgm_path, __DECONST(char *const *, args));
markj: Since you're using __DECONST below, why not here too? Actually, I think you can instead…
dnelson_1901_yahoo.comAuthorUnsubmitted
Done
Inline Actions

strdup() takes a const char* and returns a char *, so no const stripping is needed there. A better question would be "why are you strdup()'ing at all" :) It's not modified or passed anywhere. Removing that also removes the free(args[0]) call.

dnelson_1901_yahoo.com: strdup() takes a const char* and returns a char *, so no const stripping is needed there. A…
markjUnsubmitted
Not Done
Inline Actions

Right, I was wondering why you were using strdup() instead of DECONSTing.

markj: Right, I was wondering why you were using strdup() instead of DECONSTing.
for (c = 0; c < ct->nflags; c++)
args[c + 1] = ct->flags[c];
args[c + 1] = zwork->zw_fname;
markjUnsubmitted
Done
Inline Actions

Style: redundant braces.

markj: Style: redundant braces.
/* Also create a space-delimited version if we need to print it. */
if ((command = sbuf_new_auto()) == NULL)
errx(1, "sbuf_new");
sbuf_cpy(command, pgm_path);
for (c = 1; args[c] != NULL; c++) { for (c = 1; args[c] != NULL; c++) {
strlcat(command, " ", sizeof(command)); sbuf_putc(command, ' ');
strlcat(command, args[c], sizeof(command)); sbuf_cat(command, args[c]);
} }
if (sbuf_finish(command) == -1)
err(1, "sbuf_finish");
/* Determine the filename of the compressed file. */
asprintf(&zresult, "%s%s", zwork->zw_fname, ct->suffix);
if (zresult == NULL)
errx(1, "asprintf");
if (verbose)
markjUnsubmitted
Done
Inline Actions

Style: redundant braces.

markj: Style: redundant braces.
printf("Executing: %s\n", sbuf_data(command));
if (noaction) { if (noaction) {
printf("\t%s %s\n", pgm_name, zwork->zw_fname); printf("\t%s %s\n", pgm_name, zwork->zw_fname);
change_attrs(zresult, zwork->zw_conf); change_attrs(zresult, zwork->zw_conf);
return; goto out;
} }
if (verbose) {
printf("Executing: %s\n", command);
}
fcount = 1; fcount = 1;
pidzip = fork(); pidzip = fork();
while (pidzip < 0) { while (pidzip < 0) {
/* /*
* The fork failed. If the failure was due to a temporary * The fork failed. If the failure was due to a temporary
* problem, then wait a short time and try it again. * problem, then wait a short time and try it again.
*/ */
errsav = errno; errsav = errno;
warn("fork() for `%s %s'", pgm_name, zwork->zw_fname); warn("fork() for `%s %s'", pgm_name, zwork->zw_fname);
if (errsav != EAGAIN || fcount > 5) if (errsav != EAGAIN || fcount > 5)
errx(1, "Exiting..."); errx(1, "Exiting...");
sleep(fcount * 12); sleep(fcount * 12);
fcount++; fcount++;
pidzip = fork(); pidzip = fork();
} }
if (!pidzip) { if (!pidzip) {
/* The child process executes the compression command */ /* The child process executes the compression command */
execv(pgm_path, (char *const*) args); execv(pgm_path, __DECONST(char *const*, args));
err(1, "execv(`%s')", command); err(1, "execv(`%s')", sbuf_data(command));
} }
wpid = waitpid(pidzip, &zstatus, 0); wpid = waitpid(pidzip, &zstatus, 0);
if (wpid == -1) { if (wpid == -1) {
/* XXX - should this be a fatal error? */ /* XXX - should this be a fatal error? */
warn("%s: waitpid(%d)", pgm_path, pidzip); warn("%s: waitpid(%d)", pgm_path, pidzip);
return; goto out;
} }
if (!WIFEXITED(zstatus)) { if (!WIFEXITED(zstatus)) {
warnx("`%s' did not terminate normally", command); warnx("`%s' did not terminate normally", sbuf_data(command));
free(args[0]); goto out;
free(args);
return;
} }
if (WEXITSTATUS(zstatus)) { if (WEXITSTATUS(zstatus)) {
warnx("`%s' terminated with a non-zero status (%d)", command, warnx("`%s' terminated with a non-zero status (%d)",
WEXITSTATUS(zstatus)); sbuf_data(command), WEXITSTATUS(zstatus));
free(args[0]); goto out;
free(args);
return;
} }
free(args[0]);
free(args);
/* Compression was successful, set file attributes on the result. */ /* Compression was successful, set file attributes on the result. */
change_attrs(zresult, zwork->zw_conf); change_attrs(zresult, zwork->zw_conf);
out:
if (command != NULL)
sbuf_delete(command);
free(args);
free(zresult);
} }
/* /*
* Save information on any process we need to signal. Any single * Save information on any process we need to signal. Any single
* process may need to be sent different signal-values for different * process may need to be sent different signal-values for different
* log files, but usually a single signal-value will cause the process * log files, but usually a single signal-value will cause the process
* to close and re-open all of its log files. * to close and re-open all of its log files.
*/ */
▲ Show 20 Lines • Show All 290 Lines • ▼ Show 20 Lines
} }
/* Return the age in hours of the most recent archive of the logfile. */ /* Return the age in hours of the most recent archive of the logfile. */
static int static int
age_old_log(const char *file) age_old_log(const char *file)
{ {
struct stat sb; struct stat sb;
const char *logfile_suffix; const char *logfile_suffix;
char tmp[MAXPATHLEN + sizeof(".0") + COMPRESS_SUFFIX_MAXLEN + 1]; static unsigned int suffix_maxlen = 0;
char *tmp;
time_t mtime; time_t mtime;
int c;
if (suffix_maxlen == 0) {
for (c = 0; c < COMPRESS_TYPES; c++)
suffix_maxlen = MAX(suffix_maxlen,
strlen(compress_type[c].suffix));
}
tmp = alloca(MAXPATHLEN + sizeof(".0") + suffix_maxlen + 1);
markjUnsubmitted
Not Done
Inline Actions

We're building a path here... I would argue that MAXPATHLEN on its own should be sufficiently large. IMO the real issue is that this function and others are not checking for truncation.

In general alloca()'s use is discouraged.

markj: We're building a path here... I would argue that MAXPATHLEN on its own should be sufficiently…
dnelson_1901_yahoo.comAuthorUnsubmitted
Not Done
Inline Actions

Is an alloca() any worse than the current VLA usage?

I think the idea was "allocate a buffer large enough that truncation can't happen". The filename built here may be chopped up to find the names of older logfiles (which may have different or no suffixes), so even if this filename's length exceeds MAXPATHLEN, one of the older archive files might not.

get_logfile_suffix() looks like it might incorrectly match the wrong filename due to truncation, though. I'll see what can be done there.

dnelson_1901_yahoo.com: Is an alloca() any worse than the current VLA usage? I think the idea was "allocate a buffer…
markjUnsubmitted
Not Done
Inline Actions

How is it a VLA? Its size is known at compile-time.

In a common case (archtodir == 0 && timefnamefmt == NULL), we just build up the path and pass it to stat(2), which will fail if the total length exceeds MAXPATHLEN. The array size calculation is also bogus for the timefnamefmt != NULL case, where we add a suffix other than ".0". IMHO there is no reason to play these tricks to avoid truncation when the result is going to be the same anyway. If you still feel strongly that alloc() is a better way to go, I have no objection to it.

markj: How is it a VLA? Its size is known at compile-time. In a common case (archtodir == 0 &&…
dnelson_1901_yahoo.comAuthorUnsubmitted
Not Done
Inline Actions

You're right, it's a constant; the math expression fooled me. I'll leave it as an alloca() for now. Rewriting the whole thing to use an sbuf will guarantee no overflows, and should be more readable than the current code. I think that should happen in a separate patch though.

dnelson_1901_yahoo.com: You're right, it's a constant; the math expression fooled me. I'll leave it as an alloca() for…
if (archtodir) { if (archtodir) {
char *p; char *p;
/* build name of archive directory into tmp */ /* build name of archive directory into tmp */
if (*archdirname == '/') { /* absolute */ if (*archdirname == '/') { /* absolute */
strlcpy(tmp, archdirname, sizeof(tmp)); strlcpy(tmp, archdirname, sizeof(tmp));
} else { /* relative */ } else { /* relative */
▲ Show 20 Lines • Show All 332 Lines • Show Last 20 Lines