session: l_debug() params may be null need check

[GermanAizek]

affected params:

• pam_service
• username
• auth_result_string

[JPeisach]

does it crash or create bad behavior when trying to print a null? I think this may just add another condition that could slow down runtime

[GermanAizek]

does it crash or create bad behavior when trying to print a null? I think this may just add another condition that could slow down runtime

@JPeisach

l_debug() it macro this functions

void
logger_logv_default (Logger *self, GLogLevelFlags log_level, const gchar *format, va_list ap)
{
    /* figure out how long the prefix is */
    gint tmp = logger_logprefix (self, NULL, 0);
    if (tmp < 0)
    {
        g_error ("failed to get log prefix");
        return;
    }

    /* print the prefix to a variable length array (to avoid malloc) */
    gchar pfx[tmp + 1];
    tmp = logger_logprefix (self, pfx, sizeof(pfx));
    if (tmp < 0)
    {
        g_error ("failed to get log prefix");
        return;
    }

    /* figure out how long the formatted message is */
    va_list ap_copy;
    va_copy (ap_copy, ap);
    tmp = g_vsnprintf (NULL, 0, format, ap_copy);
    va_end (ap_copy);
    if (tmp < 0)
    {
        g_error ("failed to format log message");
        return;
    }

    /* print the message to a variable length array (to avoid malloc) */
    gchar msg[tmp+1];
    tmp = g_vsnprintf (msg, sizeof(msg), format, ap);
    if (tmp < 0)
    {
        g_error ("failed to format log message");
        return;
    }

    /* log the message with the prefix */
    g_log (G_LOG_DOMAIN, log_level, "%s%s", pfx, msg);
}

void
logger_log (Logger *self, GLogLevelFlags log_level, const gchar *format, ...)
{
    va_list ap;
    va_start (ap, format);
    logger_logv (self, log_level, format, ap);
    va_end (ap);
}

The format variable is passed directly to the g_vsnprintf function:

tmp = g_vsnprintf (NULL, 0, format, ap_copy);

The g_vsnprintf function (like the standard vsnprintf system, which it usually wraps around) expects the format parameter to be a valid pointer to a null-terminated string.

When trying to read a format string using a NULL pointer, the function will try to access the protected memory area. In most modern operating systems (Linux, Windows, macOS), this will cause a Segmentation fault (SIGSEGV), and the operating system will immediately "kill" the process.

[GermanAizek]

Even if we assume that your implementation of the C or GLib standard library contains internal foolproof protection and correctly returns an error (code -1) when passing NULL instead of falling, the program will still terminate.

The code has the following check:

if (tmp < 0)
{
    g_error ("failed to format log message");
    return;
}

In the GLib library, the g_error(...) call it is not a common error output. By default, this function outputs the transmitted message to the standard error stream (stderr), and then calls abort(), which also leads to an immediate hard shutdown of the application. The call to return; after g_error() will never be executed in practice.

[GermanAizek]

I think this may just add another condition that could slow down runtime

as an example, we can embed the NULL check directly in the logger functions logger_logv_default themselves. This is easier than checking all the arguments of all functions.

void
logger_logv_default (Logger *self, GLogLevelFlags log_level, const gchar *format, va_list ap)
{
    /* check format on NULL or return */
    g_return_if_fail (format != NULL); 
    
    // ...
}

I didn't notice that logger is LightDM code and not from dependencies, I should have done it right away.