Re: [HACKERS] Oops in fe-auth.c

On Mon, Jul 23, 2007 at 03:36:21PM +0200, Magnus Hagander wrote:
> I've been debugging some really weird crashes in libpq on win32, and I
> think I've finally found the reason for the heap corruption that shows up
> in msvc debug mode.
>
> When run in debug mode, the runtime for msvc will *zero-pad the entire
> buffer* in a strncpy() call. This in itself is not bad (just slow), but it
> shows a rather bad bug in libpq.
>
> In a bunch of places in fe-auth.c, we do:
> strncpy(PQerrormsg, libpq_gettext("out of memory\n"), PQERRORMSG_LENGTH);
>
>
> Except when calling it, the size of the buffer is 256 bytes. But
> PQERRORMSG_LENGTH is 1024.
>
> Naturally, this causes a heap corruption. It doesn't happen in production,
> because the string length fits as long as there is no padding.
>
> One way to get around this on win32 is to just use snprintf() instead of
> strncpy(), since it doesn't pad. But that's just hiding the underlying
> problem, so I think that's a really bad fix.
>
> I assume the comment in the header:
>  * NOTE: the error message strings returned by this module must not
>  * exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes).
>
> refers to this, but it's hard to guarantee that from the code since it's
> translated strings.
>
> I see a comment in fe-connect.c that has
> * XXX fe-auth.c has not been fixed to support PQExpBuffers,
>
>
> Given this, I'll go ahead and fix fe-connect to support PQExpBuffers,
> unless there are any objections. Also, is this something we shuold
> backpatch - or just ignore since we've had no actual reports of it in the
> field?

Actually coding up a patch for that was just a bunch of simple
search/replace ops. Attached is one that appears to work fine for me.

Actually coding up a patch for that was just a bunch of simple
search/replace ops. Attached is one that appears to work fine for me.

Was there any reason why this wasn't done before, or just nobody had the
time? If there was a reason, please let me know what it was :-) Otherwise
I'll apply this patch to fix it.

(Question about backpatch remains)

//Magnus