> > I agree with Michael in that the application's logging function should
> > accept an already-formatted string, as it would need to do some magic
> > to combine two va_list's otherwise. Unfortunately, this requires that
> > libpam either imposes a limit on the log line length, or allocates a
> > piece of memory dynamically. The latter can be done with vasprintf()
> > (non-portable) or a loop around vsnprintf() (probably acceptable).
>
> All at once seemed a bit ugly :(
> Ok, at least should have something like:
>
> char defbuf[256]; <== some not very large but still enouth
> for most cases size
> char *buf = defbuf;
> int size = sizeof(defbuf);
> while (snprintf(buf, size, ...) cant fit in size) {
The "can't fit" is ">= size" with non-broken snprintf()'s. I don't
think we should support broken ones at all, simply use a reasonable
size by default and, if that isn't enough, a truncated log entry is
the price paid for using a broken libc. ;-)
> size *= 2;
> buf = (buf == defbuf) ? malloc(size) : realloc(buf, size);
> }
Yes, this is what I meant. I don't think you need that one buffer on
the stack, -- it only adds complexity. You also need a free().
> Far more good will be to have
>
> char nfmt[strlen(format) + sizeof("%s: %s ")]; (maybe alloca'd)
> strcpy(nfmt, "%s: %s ");
> strcat(nfmt, format);
> va_prepend(args, module); \ where va_prepend() lives?! :)
> va_prepend(args, service); /
> callback(priority, module, service, nfmt, args);
>
> but that's impossible now... :(
> Maybe something similar exists?
I don't think such a thing can exist at all. The size of a va_list
isn't known, so a va_prepend() wouldn't have a way to know how much
data to copy to a new location, and there's no unused space adjacent
to the existing list.
> > int pam_log(int priority, char *format, ...)
> > {
> > char *message;
> > [...]
> > [va_start]
> > [vasprintf/vsnprintf]
> > retval = pam_log_callback(service, module, priority, message);
> > [va_end]
> > [...]
> > return retval;
> > }
>
> int pam_log(int priority, char *format, ...)
> {
> [va_start]
> retval = pam_vlog(priority, format, va_list)
> [va_end]
> return retval;
> }
>
> int pam_vlog(int priority, char *format, va_list args)
> {
> char *message;
> [...]
> [vasprintf/vsnprintf]
> retval = pam_log_callback(service, module, priority, message);
> [va_end]
> [...]
> return retval;
> }
What does this buy you? I think pam_log() can be simple enough.
> int pam_log_default(char *service, char *module, int priority, char *message)
> {
> char *locale = getlocale();
char *locale = xstrdup(setlocale(LC_ALL, NULL));
> setlocale(LC_ALL, "C");
LC_TIME only should be enough for syslog().
> openlog(service, LOG_PID, LOG_AUTH);
> syslog(priority, "%s: %s", module, message);
> closelog();
> setlocale(locale);
if (locale) {
setlocale(LC_ALL, locale);
free(locale);
}
> return 0;
> }
Yes, this is what the Shadow Suite commands do. Makes sense.
Signed,
Solar Designer
