[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: logging from PAM modules



> > 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





[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index] []