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

Re: [leafnode-list] [ANNOUNCE:] leafnode-1.9.12



Cornelius wrote:
>     if ( strlen(str) > n ) {
>         syslog( LOG_NOTICE, "snprintf buffer overflow" );
>         str[n] = '\0';
>     }

Off-by-one error there, I think: n is the total number of chars that
can be used, not a subscript, so the ='\0' will set one past the end
of the allocated space.  Also, you didn't check the return code of
vsprintf before mucking about with str.  Also, you didn't check n to
make sure it's sensible.

A string overflow can have disasterous consequences.  The Morris Worm,
for example, exploited one in the FTP daemon to make a security
breach.  In general, other data and maybe even instructions can be
trashed.  If an overflow is detected, then, I'd take immediate steps
to terminate the program, as quickly but as cleanly as possible.

Further, it's fine if a real snprintf hits n bytes; it's simply
truncated.  That doesn't warrant a message.  The problem is in this
hacked implementation, if str is overlowed.

Since the function can't know how large the str buffer is, I would
personally make a local variable that's big, like
    char bigbuf[16 * 1024 - 4];
A large local variable is usually efficient to allocate, unlike
"new".  Then, I'd check it for overflow and die, but if it's just
longer than n I'd silently truncate (unless you know leafnode ought to
break in such cases).

Solaris snprintf says that if str fills up, it's not null-terminated.
I think always null-terminating is much more useful in C/C++
programming..  I would therefore use the function unconditionally, and
call snprintf inside if it's available, and stick on the null byte at
need.

I'd definitely desk-check this again before implementing it, because
string boundary conditions are a pain and a half, but I'd do something
like this:



#include <errno.h>

// The return value differs slightly from standard snprintf.
// Solaris, for example, says
// "The vprintf(), vfprintf(), and vsprintf() functions return the
// number of characters transmitted (not including \0 in the case of
// vsprintf()).  The vsnprintf() function returns the number of
// characters formatted, that is, the number of characters that would
// have been written to the buffer if it were large enough."
// This is ambigious: it doesn't say whether vsnprintf includes \0 or
// no.  It also doesn't null-terminate str if it fills.
//
// This version always null-terminates the result.  If there was no
// error, this version simply returns strlen(str), which is guaranteed
// to be < n; otherwise, it returns what vsnprintf returns if that
// function is implemented on the computer.
//
int leafnode_snprintf(char *str, size_t n, const char *fmt, ...) {
#if !defined(HAVE_SNPRINTF)
    char bigbuf[16 * 1024 - 4];
    size_t len;
#endif
    int rval;
    va_list ap;

    va_start(ap, fmt);
#if defined(HAVE_SNPRINTF)
    rval = vsnprintf(str, n, fmt, ap);
    // Here, we don't depend on the unclear return value, except
    // that <0 means "error".
    if (rval >= 0 && n >= 1) {
        str[n - 1] = '\0';
        rval = strlen(str);
    }
#else
    if (n <= 0) {
        // then there's no room for chars, so we can't touch str.
        // This is returned as an error, not just "rval = 0;", because
        // we guarantee that for any normal return, str is
        // null-terminated, and in this case it isn't.
        // Any sane caller will pass in "sizeof something" as n,
        // so we hope this never happens.
        rval = -1;
        errno = EINVAL;
    } else {
        rval = vsprintf(bigbuf, fmt, ap);

        if (rval >= 0) {
            len = strlen(bigbuf);
            if ( len > sizeof[bigbuf] - 1 ) {
                // Buffer overflow: other memory has been corrupted.
                FIXME;
                // die painfully and obviously in a way appropriate to leafnode
            }
            // We know n>=1, so n-1>=0, so no wraparound / bad subscript.
            if (len > n - 1) {
                len = n - 1;
            }
            // Now (len == min(len, n-1)).
            bigbuf[len] = '\0';
            strcpy(str, bigbuf);
            rval = len;
        } // if (rval >= 0)
    } // if (n <= 0)
#endif
    return rval;
}



-- 
Tim McDaniel is tmcd@xxxxxxxx; if that fail,
    tmcd@xxxxxxxxxx is my work account.
"To join the Clueless Club, send a followup to this message quoting everything
up to and including this sig!" -- Jukka.Korpela@xxxxxx (Jukka Korpela)

-- 
leafnode-list@xxxxxxxxxxxxxxxxxxxxxxxxxxxx -- mailing list for leafnode
To unsubscribe, send mail with "unsubscribe" in the subject to the list