Potential snprintf buffer overflow

[Fish-Git]

The following construct -- used in many different places all throughout Hercules -- can easily result in a buffer overflow if the size of the initial buffer is too small:

    n += snprintf( buf+n, bufl-n, "format pattern...", variables, ... );

In the typical Hercules use case, n is typically an int that is incremented based on the return value from snprintf, and bufl is typicall defined as a size_t.

As a crude example, if the buffer is initially defined as being only 8 characters long (i.e. char buf[8]), then the following would result in an obvious buffer overflow:

    n += snprintf( buf+n, bufl-n, "%s", "example of " );
    n += snprintf( buf+n, bufl-n, "%s", "buffer overflow!" );

This is because the return value from snprintf is defined as:

Upon successful completion, the snprintf() function shall return the number of bytes that would be written to s had n been sufficiently large excluding the terminating null byte.

I have not checked every place where we perform such series of snprintfs to ensure the initial buffer size is sufficiently large enough to prevent a buffer overflow from occurring, but IMO we probably shouldn't be relying on such things. Instead, I'm thinking we should define a new function (possibly called idx_snprintf) who's first argument is the int index into the original buffer, and where the buffer and buffer size passed is not offset by the index. For example:

 int idx_snprintf( int idx, char *buf, size_t bufl, const char *fmt, ... );

The idx_snprintf function can then do the snprintf using buf+n and bufl-n but only after first verifying that n is <= bufl!. If n is > bufl, then the function can issue a diagnostic message and immediately return 0 without doing anything.

Then when we do:

    n += idx_snprintf( n, buf, bufl, "%s", "this would " );
    n += idx_snprintf( n, buf, bufl, "%s", "not overflow." );

no buffer overflow would occur (and we'd get a diagnostic letting us know we need to increase the size of our buffer).

Thoughts?

[s390guy]

Clearly the right thing to do. Devilish details to be discovered.

[Fish-Git]

Closed by commit 6401129ab47272b559e8fb0787166ec030f6ffce.

[s390guy]

Sneaky devil. Already had the changes ready to go.

[Fish-Git]

Sneaky devil. Already had the changes ready to go.

Nope. The only thing I did ahead of time was to identify the problem and propose the fix for it (i.e. create the GitHub Issue for it). The actual effort to actually fix the problem didn't begin until the moment I added the "IN PROGRESS..." label sometime yesterday. It was a pretty easy fix. I admittedly should have done more. There are still a few places where a buffer overflow is still possible if we ever decide to print significantly more data than what is currently being printed, but being the lazy fucker I am I didn't bother fixing all of those. I just wanted to fix the specific problem as described. (Which, as I said, didn't take much time/effort at all to complete.)