Oops in fe-auth.c
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?
//Magnus
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
Attachments:
libpq.difftext/plain; charset=us-asciiDownload
Index: src/interfaces/libpq/fe-auth.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.129
diff -c -r1.129 fe-auth.c
*** src/interfaces/libpq/fe-auth.c 23 Jul 2007 10:57:36 -0000 1.129
--- src/interfaces/libpq/fe-auth.c 23 Jul 2007 14:02:28 -0000
***************
*** 6,14 ****
* Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * NOTE: the error message strings returned by this module must not
- * exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes).
- *
* IDENTIFICATION
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-auth.c,v 1.129 2007/07/23 10:57:36 mha Exp $
*
--- 6,11 ----
***************
*** 131,137 ****
static int
! pg_krb5_init(char *PQerrormsg, struct krb5_info * info)
{
krb5_error_code retval;
--- 128,134 ----
static int
! pg_krb5_init(PQExpBuffer errorMessage, struct krb5_info * info)
{
krb5_error_code retval;
***************
*** 141,147 ****
retval = krb5_init_context(&(info->pg_krb5_context));
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"pg_krb5_init: krb5_init_context: %s\n",
error_message(retval));
return STATUS_ERROR;
--- 138,144 ----
retval = krb5_init_context(&(info->pg_krb5_context));
if (retval)
{
! printfPQExpBuffer(errorMessage,
"pg_krb5_init: krb5_init_context: %s\n",
error_message(retval));
return STATUS_ERROR;
***************
*** 150,156 ****
retval = krb5_cc_default(info->pg_krb5_context, &(info->pg_krb5_ccache));
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"pg_krb5_init: krb5_cc_default: %s\n",
error_message(retval));
krb5_free_context(info->pg_krb5_context);
--- 147,153 ----
retval = krb5_cc_default(info->pg_krb5_context, &(info->pg_krb5_ccache));
if (retval)
{
! printfPQExpBuffer(errorMessage,
"pg_krb5_init: krb5_cc_default: %s\n",
error_message(retval));
krb5_free_context(info->pg_krb5_context);
***************
*** 161,167 ****
&(info->pg_krb5_client));
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"pg_krb5_init: krb5_cc_get_principal: %s\n",
error_message(retval));
krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache);
--- 158,164 ----
&(info->pg_krb5_client));
if (retval)
{
! printfPQExpBuffer(errorMessage,
"pg_krb5_init: krb5_cc_get_principal: %s\n",
error_message(retval));
krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache);
***************
*** 172,178 ****
retval = krb5_unparse_name(info->pg_krb5_context, info->pg_krb5_client, &(info->pg_krb5_name));
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"pg_krb5_init: krb5_unparse_name: %s\n",
error_message(retval));
krb5_free_principal(info->pg_krb5_context, info->pg_krb5_client);
--- 169,175 ----
retval = krb5_unparse_name(info->pg_krb5_context, info->pg_krb5_client, &(info->pg_krb5_name));
if (retval)
{
! printfPQExpBuffer(errorMessage,
"pg_krb5_init: krb5_unparse_name: %s\n",
error_message(retval));
krb5_free_principal(info->pg_krb5_context, info->pg_krb5_client);
***************
*** 203,216 ****
* has authenticated to the system, or NULL
*/
static char *
! pg_krb5_authname(char *PQerrormsg)
{
char *tmp_name;
struct krb5_info info;
info.pg_krb5_initialised = 0;
! if (pg_krb5_init(PQerrormsg, &info) != STATUS_OK)
return NULL;
tmp_name = strdup(info.pg_krb5_name);
pg_krb5_destroy(&info);
--- 200,213 ----
* has authenticated to the system, or NULL
*/
static char *
! pg_krb5_authname(PQExpBuffer errorMessage)
{
char *tmp_name;
struct krb5_info info;
info.pg_krb5_initialised = 0;
! if (pg_krb5_init(errorMessage, &info) != STATUS_OK)
return NULL;
tmp_name = strdup(info.pg_krb5_name);
pg_krb5_destroy(&info);
***************
*** 224,230 ****
* the server
*/
static int
! pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *servicename)
{
krb5_error_code retval;
int ret;
--- 221,227 ----
* the server
*/
static int
! pg_krb5_sendauth(PGconn *conn, int sock, const char *hostname, const char *servicename)
{
krb5_error_code retval;
int ret;
***************
*** 237,248 ****
if (!hostname)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"pg_krb5_sendauth: hostname must be specified for Kerberos authentication\n");
return STATUS_ERROR;
}
! ret = pg_krb5_init(PQerrormsg, &info);
if (ret != STATUS_OK)
return ret;
--- 234,245 ----
if (!hostname)
{
! printfPQExpBuffer(&conn->errorMessage,
"pg_krb5_sendauth: hostname must be specified for Kerberos authentication\n");
return STATUS_ERROR;
}
! ret = pg_krb5_init(&conn->errorMessage, &info);
if (ret != STATUS_OK)
return ret;
***************
*** 250,256 ****
KRB5_NT_SRV_HST, &server);
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"pg_krb5_sendauth: krb5_sname_to_principal: %s\n",
error_message(retval));
pg_krb5_destroy(&info);
--- 247,253 ----
KRB5_NT_SRV_HST, &server);
if (retval)
{
! printfPQExpBuffer(&conn->errorMessage,
"pg_krb5_sendauth: krb5_sname_to_principal: %s\n",
error_message(retval));
pg_krb5_destroy(&info);
***************
*** 266,272 ****
{
char sebuf[256];
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("could not set socket to blocking mode: %s\n"), pqStrerror(errno, sebuf, sizeof(sebuf)));
krb5_free_principal(info.pg_krb5_context, server);
pg_krb5_destroy(&info);
--- 263,269 ----
{
char sebuf[256];
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not set socket to blocking mode: %s\n"), pqStrerror(errno, sebuf, sizeof(sebuf)));
krb5_free_principal(info.pg_krb5_context, server);
pg_krb5_destroy(&info);
***************
*** 284,294 ****
if (retval == KRB5_SENDAUTH_REJECTED && err_ret)
{
#if defined(HAVE_KRB5_ERROR_TEXT_DATA)
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("Kerberos 5 authentication rejected: %*s\n"),
(int) err_ret->text.length, err_ret->text.data);
#elif defined(HAVE_KRB5_ERROR_E_DATA)
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("Kerberos 5 authentication rejected: %*s\n"),
(int) err_ret->e_data->length,
(const char *) err_ret->e_data->data);
--- 281,291 ----
if (retval == KRB5_SENDAUTH_REJECTED && err_ret)
{
#if defined(HAVE_KRB5_ERROR_TEXT_DATA)
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("Kerberos 5 authentication rejected: %*s\n"),
(int) err_ret->text.length, err_ret->text.data);
#elif defined(HAVE_KRB5_ERROR_E_DATA)
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("Kerberos 5 authentication rejected: %*s\n"),
(int) err_ret->e_data->length,
(const char *) err_ret->e_data->data);
***************
*** 298,304 ****
}
else
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"krb5_sendauth: %s\n", error_message(retval));
}
--- 295,301 ----
}
else
{
! printfPQExpBuffer(&conn->errorMessage,
"krb5_sendauth: %s\n", error_message(retval));
}
***************
*** 314,320 ****
{
char sebuf[256];
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("could not restore non-blocking mode on socket: %s\n"),
pqStrerror(errno, sebuf, sizeof(sebuf)));
ret = STATUS_ERROR;
--- 311,317 ----
{
char sebuf[256];
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not restore non-blocking mode on socket: %s\n"),
pqStrerror(errno, sebuf, sizeof(sebuf)));
ret = STATUS_ERROR;
***************
*** 374,399 ****
* both parts into the string.
*/
static void
! pg_GSS_error(char *mprefix, char *msg, int msglen,
OM_uint32 maj_stat, OM_uint32 min_stat)
{
int mlen;
/* Fetch major error codes */
! pg_GSS_error_int(mprefix, msg, msglen, maj_stat, GSS_C_GSS_CODE);
! mlen = strlen(msg);
/* If there is room left, try to add the minor codes as well */
! if (mlen < msglen-1)
! pg_GSS_error_int(mprefix, msg + mlen, msglen - mlen,
! min_stat, GSS_C_MECH_CODE);
}
/*
* Continue GSS authentication with next token as needed.
*/
static int
! pg_GSS_continue(char *PQerrormsg, PGconn *conn)
{
OM_uint32 maj_stat, min_stat, lmin_s;
--- 371,397 ----
* both parts into the string.
*/
static void
! pg_GSS_error(char *mprefix, PGconn *conn,
OM_uint32 maj_stat, OM_uint32 min_stat)
{
int mlen;
/* Fetch major error codes */
! pg_GSS_error_int(mprefix, conn->errorMessage.data,
! conn->errorMessage.maxlen, maj_stat, GSS_C_GSS_CODE);
! mlen = strlen(conn->errorMessage.data);
/* If there is room left, try to add the minor codes as well */
! if (mlen < conn->errorMessage.maxlen - 1)
! pg_GSS_error_int(mprefix, conn->errorMessage.data + mlen,
! conn->errorMessage.maxlen - mlen, min_stat, GSS_C_MECH_CODE);
}
/*
* Continue GSS authentication with next token as needed.
*/
static int
! pg_GSS_continue(PGconn *conn)
{
OM_uint32 maj_stat, min_stat, lmin_s;
***************
*** 438,444 ****
if (maj_stat != GSS_S_COMPLETE && maj_stat != GSS_S_CONTINUE_NEEDED)
{
pg_GSS_error(libpq_gettext("GSSAPI continuation error"),
! PQerrormsg, PQERRORMSG_LENGTH,
maj_stat, min_stat);
gss_release_name(&lmin_s, &conn->gtarg_nam);
if (conn->gctx)
--- 436,442 ----
if (maj_stat != GSS_S_COMPLETE && maj_stat != GSS_S_CONTINUE_NEEDED)
{
pg_GSS_error(libpq_gettext("GSSAPI continuation error"),
! conn,
maj_stat, min_stat);
gss_release_name(&lmin_s, &conn->gtarg_nam);
if (conn->gctx)
***************
*** 456,462 ****
* Send initial GSS authentication token
*/
static int
! pg_GSS_startup(char *PQerrormsg, PGconn *conn)
{
OM_uint32 maj_stat, min_stat;
int maxlen;
--- 454,460 ----
* Send initial GSS authentication token
*/
static int
! pg_GSS_startup(PGconn *conn)
{
OM_uint32 maj_stat, min_stat;
int maxlen;
***************
*** 464,470 ****
if (conn->gctx)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("duplicate GSS auth request\n"));
return STATUS_ERROR;
}
--- 462,468 ----
if (conn->gctx)
{
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("duplicate GSS auth request\n"));
return STATUS_ERROR;
}
***************
*** 486,492 ****
if (maj_stat != GSS_S_COMPLETE)
{
pg_GSS_error(libpq_gettext("GSSAPI name import error"),
! PQerrormsg, PQERRORMSG_LENGTH,
maj_stat, min_stat);
return STATUS_ERROR;
}
--- 484,490 ----
if (maj_stat != GSS_S_COMPLETE)
{
pg_GSS_error(libpq_gettext("GSSAPI name import error"),
! conn,
maj_stat, min_stat);
return STATUS_ERROR;
}
***************
*** 497,503 ****
*/
conn->gctx = GSS_C_NO_CONTEXT;
! return pg_GSS_continue(PQerrormsg, conn);
}
#endif /* ENABLE_GSS */
--- 495,501 ----
*/
conn->gctx = GSS_C_NO_CONTEXT;
! return pg_GSS_continue(conn);
}
#endif /* ENABLE_GSS */
***************
*** 508,528 ****
*/
static void
! pg_SSPI_error(char *mprefix, char *msg, int msglen, SECURITY_STATUS r)
{
char sysmsg[256];
if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0, sysmsg, sizeof(sysmsg), NULL) == 0)
! snprintf(msg, msglen, "%s: sspi error %x", mprefix, r);
else
! snprintf(msg, msglen, "%s: %s (%x)", mprefix, sysmsg, r);
}
/*
* Continue SSPI authentication with next token as needed.
*/
static int
! pg_SSPI_continue(char *PQerrormsg, PGconn *conn)
{
SECURITY_STATUS r;
CtxtHandle newContext;
--- 506,526 ----
*/
static void
! pg_SSPI_error(PGconn *conn, char *mprefix, SECURITY_STATUS r)
{
char sysmsg[256];
if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0, sysmsg, sizeof(sysmsg), NULL) == 0)
! snprintf(conn->errorMessage.data, conn->errorMessage.maxlen, "%s: sspi error %x", mprefix, r);
else
! snprintf(conn->errorMessage.data, conn->errorMessage.maxlen, "%s: %s (%x)", mprefix, sysmsg, r);
}
/*
* Continue SSPI authentication with next token as needed.
*/
static int
! pg_SSPI_continue(PGconn *conn)
{
SECURITY_STATUS r;
CtxtHandle newContext;
***************
*** 568,575 ****
if (r != SEC_E_OK && r != SEC_I_CONTINUE_NEEDED)
{
! pg_SSPI_error(libpq_gettext("SSPI continuation error"),
! PQerrormsg, PQERRORMSG_LENGTH, r);
return STATUS_ERROR;
}
--- 566,572 ----
if (r != SEC_E_OK && r != SEC_I_CONTINUE_NEEDED)
{
! pg_SSPI_error(conn, libpq_gettext("SSPI continuation error"), r);
return STATUS_ERROR;
}
***************
*** 580,586 ****
conn->sspictx = malloc(sizeof(CtxtHandle));
if (conn->sspictx == NULL)
{
! strncpy(PQerrormsg, libpq_gettext("out of memory\n"), PQERRORMSG_LENGTH);
return STATUS_ERROR;
}
memcpy(conn->sspictx, &newContext, sizeof(CtxtHandle));
--- 577,583 ----
conn->sspictx = malloc(sizeof(CtxtHandle));
if (conn->sspictx == NULL)
{
! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("out of memory\n"));
return STATUS_ERROR;
}
memcpy(conn->sspictx, &newContext, sizeof(CtxtHandle));
***************
*** 608,614 ****
* This should never happen, at least not for Kerberos authentication. Keep check
* in case it shows up with other authentication methods later.
*/
! strncpy(PQerrormsg, "SSPI returned invalid number of output buffers\n", PQERRORMSG_LENGTH);
return STATUS_ERROR;
}
--- 605,611 ----
* This should never happen, at least not for Kerberos authentication. Keep check
* in case it shows up with other authentication methods later.
*/
! printfPQExpBuffer(&conn->errorMessage, "SSPI returned invalid number of output buffers\n");
return STATUS_ERROR;
}
***************
*** 632,638 ****
* which supports both kerberos and NTLM, but is not compatible with Unix.
*/
static int
! pg_SSPI_startup(char *PQerrormsg, PGconn *conn, int use_negotiate)
{
SECURITY_STATUS r;
TimeStamp expire;
--- 629,635 ----
* which supports both kerberos and NTLM, but is not compatible with Unix.
*/
static int
! pg_SSPI_startup(PGconn *conn, int use_negotiate)
{
SECURITY_STATUS r;
TimeStamp expire;
***************
*** 645,658 ****
conn->sspicred = malloc(sizeof(CredHandle));
if (conn->sspicred == NULL)
{
! strncpy(PQerrormsg, libpq_gettext("out of memory\n"), PQERRORMSG_LENGTH);
return STATUS_ERROR;
}
r = AcquireCredentialsHandle(NULL, use_negotiate?"negotiate":"kerberos", SECPKG_CRED_OUTBOUND, NULL, NULL, NULL, NULL, conn->sspicred, &expire);
if (r != SEC_E_OK)
{
! pg_SSPI_error("acquire credentials failed", PQerrormsg, PQERRORMSG_LENGTH, r);
free(conn->sspicred);
conn->sspicred = NULL;
return STATUS_ERROR;
--- 642,655 ----
conn->sspicred = malloc(sizeof(CredHandle));
if (conn->sspicred == NULL)
{
! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("out of memory\n"));
return STATUS_ERROR;
}
r = AcquireCredentialsHandle(NULL, use_negotiate?"negotiate":"kerberos", SECPKG_CRED_OUTBOUND, NULL, NULL, NULL, NULL, conn->sspicred, &expire);
if (r != SEC_E_OK)
{
! pg_SSPI_error(conn, "acquire credentials failed", r);
free(conn->sspicred);
conn->sspicred = NULL;
return STATUS_ERROR;
***************
*** 665,677 ****
*/
if (conn->pghost == NULL)
{
! strncpy(PQerrormsg, libpq_gettext("hostname must be specified\n"), PQERRORMSG_LENGTH);
return STATUS_ERROR;
}
conn->sspitarget = malloc(strlen(conn->krbsrvname)+strlen(conn->pghost)+2);
if (!conn->sspitarget)
{
! strncpy(PQerrormsg, libpq_gettext("out of memory\n"), PQERRORMSG_LENGTH);
return STATUS_ERROR;
}
sprintf(conn->sspitarget, "%s/%s", conn->krbsrvname, conn->pghost);
--- 662,674 ----
*/
if (conn->pghost == NULL)
{
! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("hostname must be specified\n"));
return STATUS_ERROR;
}
conn->sspitarget = malloc(strlen(conn->krbsrvname)+strlen(conn->pghost)+2);
if (!conn->sspitarget)
{
! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("out of memory\n"));
return STATUS_ERROR;
}
sprintf(conn->sspitarget, "%s/%s", conn->krbsrvname, conn->pghost);
***************
*** 682,688 ****
*/
conn->usesspi = 1;
! return pg_SSPI_continue(PQerrormsg, conn);
}
#endif /* ENABLE_SSPI */
--- 679,685 ----
*/
conn->usesspi = 1;
! return pg_SSPI_continue(conn);
}
#endif /* ENABLE_SSPI */
***************
*** 694,700 ****
* code anyway.
*/
static int
! pg_local_sendauth(char *PQerrormsg, PGconn *conn)
{
#if defined(HAVE_STRUCT_CMSGCRED) || defined(HAVE_STRUCT_FCRED) || \
(defined(HAVE_STRUCT_SOCKCRED) && defined(LOCAL_CREDS))
--- 691,697 ----
* code anyway.
*/
static int
! pg_local_sendauth(PGconn *conn)
{
#if defined(HAVE_STRUCT_CMSGCRED) || defined(HAVE_STRUCT_FCRED) || \
(defined(HAVE_STRUCT_SOCKCRED) && defined(LOCAL_CREDS))
***************
*** 736,749 ****
{
char sebuf[256];
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"pg_local_sendauth: sendmsg: %s\n",
pqStrerror(errno, sebuf, sizeof(sebuf)));
return STATUS_ERROR;
}
return STATUS_OK;
#else
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("SCM_CRED authentication method not supported\n"));
return STATUS_ERROR;
#endif
--- 733,746 ----
{
char sebuf[256];
! printfPQExpBuffer(&conn->errorMessage,
"pg_local_sendauth: sendmsg: %s\n",
pqStrerror(errno, sebuf, sizeof(sebuf)));
return STATUS_ERROR;
}
return STATUS_OK;
#else
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SCM_CRED authentication method not supported\n"));
return STATUS_ERROR;
#endif
***************
*** 817,823 ****
*/
int
pg_fe_sendauth(AuthRequest areq, PGconn *conn, const char *hostname,
! const char *password, char *PQerrormsg)
{
#ifndef KRB5
(void) hostname; /* not used */
--- 814,820 ----
*/
int
pg_fe_sendauth(AuthRequest areq, PGconn *conn, const char *hostname,
! const char *password)
{
#ifndef KRB5
(void) hostname; /* not used */
***************
*** 829,852 ****
break;
case AUTH_REQ_KRB4:
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("Kerberos 4 authentication not supported\n"));
return STATUS_ERROR;
case AUTH_REQ_KRB5:
#ifdef KRB5
pglock_thread();
! if (pg_krb5_sendauth(PQerrormsg, conn->sock,
hostname, conn->krbsrvname) != STATUS_OK)
{
! /* PQerrormsg already filled in */
pgunlock_thread();
return STATUS_ERROR;
}
pgunlock_thread();
break;
#else
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("Kerberos 5 authentication not supported\n"));
return STATUS_ERROR;
#endif
--- 826,849 ----
break;
case AUTH_REQ_KRB4:
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("Kerberos 4 authentication not supported\n"));
return STATUS_ERROR;
case AUTH_REQ_KRB5:
#ifdef KRB5
pglock_thread();
! if (pg_krb5_sendauth(conn, conn->sock,
hostname, conn->krbsrvname) != STATUS_OK)
{
! /* Error message already filled in */
pgunlock_thread();
return STATUS_ERROR;
}
pgunlock_thread();
break;
#else
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("Kerberos 5 authentication not supported\n"));
return STATUS_ERROR;
#endif
***************
*** 865,881 ****
*/
#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
if (conn->gsslib && (pg_strcasecmp(conn->gsslib, "gssapi") == 0))
! r = pg_GSS_startup(PQerrormsg, conn);
else
! r = pg_SSPI_startup(PQerrormsg, conn, 0);
#elif defined(ENABLE_GSS) && !defined(ENABLE_SSPI)
! r = pg_GSS_startup(PQerrormsg, conn);
#elif !defined(ENABLE_GSS) && defined(ENABLE_SSPI)
! r = pg_SSPI_startup(PQerrormsg, conn, 0);
#endif
if (r != STATUS_OK)
{
! /* PQerrormsg already filled in. */
pgunlock_thread();
return STATUS_ERROR;
}
--- 862,878 ----
*/
#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
if (conn->gsslib && (pg_strcasecmp(conn->gsslib, "gssapi") == 0))
! r = pg_GSS_startup(conn);
else
! r = pg_SSPI_startup(conn, 0);
#elif defined(ENABLE_GSS) && !defined(ENABLE_SSPI)
! r = pg_GSS_startup(conn);
#elif !defined(ENABLE_GSS) && defined(ENABLE_SSPI)
! r = pg_SSPI_startup(conn, 0);
#endif
if (r != STATUS_OK)
{
! /* Error message already filled in. */
pgunlock_thread();
return STATUS_ERROR;
}
***************
*** 889,905 ****
pglock_thread();
#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
if (conn->usesspi)
! r = pg_SSPI_continue(PQerrormsg, conn);
else
! r = pg_GSS_continue(PQerrormsg, conn);
#elif defined(ENABLE_GSS) && !defined(ENABLE_SSPI)
! r = pg_GSS_continue(PQerrormsg, conn);
#elif !defined(ENABLE_GSS) && defined(ENABLE_SSPI)
! r = pg_SSPI_continue(PQerrormsg, conn);
#endif
if (r != STATUS_OK)
{
! /* PQerrormsg already filled in. */
pgunlock_thread();
return STATUS_ERROR;
}
--- 886,902 ----
pglock_thread();
#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
if (conn->usesspi)
! r = pg_SSPI_continue(conn);
else
! r = pg_GSS_continue(conn);
#elif defined(ENABLE_GSS) && !defined(ENABLE_SSPI)
! r = pg_GSS_continue(conn);
#elif !defined(ENABLE_GSS) && defined(ENABLE_SSPI)
! r = pg_SSPI_continue(conn);
#endif
if (r != STATUS_OK)
{
! /* Error message already filled in. */
pgunlock_thread();
return STATUS_ERROR;
}
***************
*** 910,916 ****
#else
case AUTH_REQ_GSS:
case AUTH_REQ_GSS_CONT:
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("GSSAPI authentication not supported\n"));
return STATUS_ERROR;
#endif
--- 907,913 ----
#else
case AUTH_REQ_GSS:
case AUTH_REQ_GSS_CONT:
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("GSSAPI authentication not supported\n"));
return STATUS_ERROR;
#endif
***************
*** 923,931 ****
* SSPI negotiation instead of Kerberos.
*/
pglock_thread();
! if (pg_SSPI_startup(PQerrormsg, conn, 1) != STATUS_OK)
{
! /* PQerrormsg already filled in. */
pgunlock_thread();
return STATUS_ERROR;
}
--- 920,928 ----
* SSPI negotiation instead of Kerberos.
*/
pglock_thread();
! if (pg_SSPI_startup(conn, 1) != STATUS_OK)
{
! /* Error message already filled in. */
pgunlock_thread();
return STATUS_ERROR;
}
***************
*** 933,939 ****
break;
#else
case AUTH_REQ_SSPI:
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("SSPI authentication not supported\n"));
return STATUS_ERROR;
#endif
--- 930,936 ----
break;
#else
case AUTH_REQ_SSPI:
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSPI authentication not supported\n"));
return STATUS_ERROR;
#endif
***************
*** 944,968 ****
case AUTH_REQ_PASSWORD:
if (password == NULL || *password == '\0')
{
! (void) snprintf(PQerrormsg, PQERRORMSG_LENGTH,
PQnoPasswordSupplied);
return STATUS_ERROR;
}
if (pg_password_sendauth(conn, password, areq) != STATUS_OK)
{
! (void) snprintf(PQerrormsg, PQERRORMSG_LENGTH,
"fe_sendauth: error sending password authentication\n");
return STATUS_ERROR;
}
break;
case AUTH_REQ_SCM_CREDS:
! if (pg_local_sendauth(PQerrormsg, conn) != STATUS_OK)
return STATUS_ERROR;
break;
default:
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext("authentication method %u not supported\n"), areq);
return STATUS_ERROR;
}
--- 941,965 ----
case AUTH_REQ_PASSWORD:
if (password == NULL || *password == '\0')
{
! printfPQExpBuffer(&conn->errorMessage,
PQnoPasswordSupplied);
return STATUS_ERROR;
}
if (pg_password_sendauth(conn, password, areq) != STATUS_OK)
{
! printfPQExpBuffer(&conn->errorMessage,
"fe_sendauth: error sending password authentication\n");
return STATUS_ERROR;
}
break;
case AUTH_REQ_SCM_CREDS:
! if (pg_local_sendauth(conn) != STATUS_OK)
return STATUS_ERROR;
break;
default:
! printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("authentication method %u not supported\n"), areq);
return STATUS_ERROR;
}
***************
*** 975,984 ****
* pg_fe_getauthname -- returns a pointer to dynamic space containing whatever
* name the user has authenticated to the system
*
! * if there is an error, return NULL with an error message in PQerrormsg
*/
char *
! pg_fe_getauthname(char *PQerrormsg)
{
#ifdef KRB5
char *krb5_name = NULL;
--- 972,981 ----
* pg_fe_getauthname -- returns a pointer to dynamic space containing whatever
* name the user has authenticated to the system
*
! * if there is an error, return NULL with an error message in pg_conn
*/
char *
! pg_fe_getauthname(PQExpBuffer errorMessage)
{
#ifdef KRB5
char *krb5_name = NULL;
***************
*** 1013,1019 ****
* however, we don't want to free 'name' directly in case it's *not* a
* Kerberos login and we fall through to name = pw->pw_name;
*/
! krb5_name = pg_krb5_authname(PQerrormsg);
name = krb5_name;
#endif
--- 1010,1016 ----
* however, we don't want to free 'name' directly in case it's *not* a
* Kerberos login and we fall through to name = pw->pw_name;
*/
! krb5_name = pg_krb5_authname(errorMessage);
name = krb5_name;
#endif
Index: src/interfaces/libpq/fe-auth.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-auth.h,v
retrieving revision 1.26
diff -c -r1.26 fe-auth.h
*** src/interfaces/libpq/fe-auth.h 5 Jan 2007 22:20:00 -0000 1.26
--- src/interfaces/libpq/fe-auth.h 23 Jul 2007 13:49:02 -0000
***************
*** 19,25 ****
extern int pg_fe_sendauth(AuthRequest areq, PGconn *conn, const char *hostname,
! const char *password, char *PQerrormsg);
! extern char *pg_fe_getauthname(char *PQerrormsg);
#endif /* FE_AUTH_H */
--- 19,25 ----
extern int pg_fe_sendauth(AuthRequest areq, PGconn *conn, const char *hostname,
! const char *password);
! extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
#endif /* FE_AUTH_H */
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.350
diff -c -r1.350 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 23 Jul 2007 10:16:54 -0000 1.350
--- src/interfaces/libpq/fe-connect.c 23 Jul 2007 13:56:44 -0000
***************
*** 1723,1734 ****
* avoid the Kerberos code doing a hostname look-up.
*/
! /*
! * XXX fe-auth.c has not been fixed to support PQExpBuffers,
! * so:
! */
! if (pg_fe_sendauth(areq, conn, conn->pghost, conn->pgpass,
! conn->errorMessage.data) != STATUS_OK)
{
conn->errorMessage.len = strlen(conn->errorMessage.data);
goto error_return;
--- 1723,1730 ----
* avoid the Kerberos code doing a hostname look-up.
*/
! if (pg_fe_sendauth(areq, conn, conn->pghost, conn->pgpass)
! != STATUS_OK)
{
conn->errorMessage.len = strlen(conn->errorMessage.data);
goto error_return;
***************
*** 3074,3080 ****
char *cp2;
PQconninfoOption *options;
PQconninfoOption *option;
- char errortmp[PQERRORMSG_LENGTH];
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
--- 3070,3075 ----
***************
*** 3297,3304 ****
*/
if (strcmp(option->keyword, "user") == 0)
{
! option->val = pg_fe_getauthname(errortmp);
! /* note any error message is thrown away */
continue;
}
}
--- 3292,3298 ----
*/
if (strcmp(option->keyword, "user") == 0)
{
! option->val = pg_fe_getauthname(errorMessage);
continue;
}
}
Magnus Hagander <magnus@hagander.net> writes:
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.
[squint] That is the specified behavior of strncpy on every platform,
not only msvc. If there's a bug here why didn't we notice it long ago?
Given this, I'll go ahead and fix fe-connect to support PQExpBuffers,
unless there are any objections.
I'm not against that, but I question what bug you've really found.
regards, tom lane
On Mon, Jul 23, 2007 at 10:28:57AM -0400, Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
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.[squint] That is the specified behavior of strncpy on every platform,
not only msvc. If there's a bug here why didn't we notice it long ago?
Hmm. Interesting - I see that now if I look at
http://www.opengroup.org/onlinepubs/007908799/xsh/strncpy.html.
That's very interesting - but my debugger very much shows me that the
buffer size is 256 bytes (INITIAL_EXPBUFFER_SIZE), and passes
1024 (PQERRORMSG_LENGTH) as the size of the buffer...
Perhaps we've just never hit one of those codepaths before. Previously, it
was only used for out of memory errors - the gssapi code adds a few places
where it's used in other cases, and this is where it crashed for me.
Given this, I'll go ahead and fix fe-connect to support PQExpBuffers,
unless there are any objections.I'm not against that, but I question what bug you've really found.
I never actually tested if it crashes on mingw, but looking some more at it
it really should - once one of these errors happen.
//Magnus
Magnus Hagander wrote:
On Mon, Jul 23, 2007 at 10:28:57AM -0400, Tom Lane wrote:
Given this, I'll go ahead and fix fe-connect to support PQExpBuffers,
unless there are any objections.I'm not against that, but I question what bug you've really found.
I never actually tested if it crashes on mingw, but looking some more at it
it really should - once one of these errors happen.
Hm. Much easier than that - the code is new in HEAD. 8.2 did
fprintf(stderr). And HEAD still does that in at least one case.
Anyway, I'll go ahead with the patch I wrote since it does Seem Nicer to
actually use the PQexpbuffer code there, and the patch was rather
trivial, but it's certainly not something to backpatch then...
I also found at least one other place in libpq where it still does
fprintf(stderr). That should probably be fixed at the same time, right?
//Magnus
Magnus Hagander <magnus@hagander.net> writes:
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 :-)
AFAIR nobody got round to it because it hadn't seemed important.
(Question about backpatch remains)
I'd vote against backpatching. The appropriate fix for back branches
is probably just to reduce the strncpy and snprintf arguments to
INITIAL_EXPBUFFER_SIZE, ie, make the code do what that header comment
says it should do.
Style point: in the places where you've chosen to pass the whole PGconn,
you should remove any separate arguments that are actually just PGconn
fields; eg for pg_krb5_sendauth it looks like sock and servicename are
now redundant. Otherwise there are risks of programmer confusion, and
maybe even wrong code generation, due to aliasing.
It would be more consistent to pass PGconn around to all of these
functions instead of trying to have them have just partial views of it,
but I dunno if you want to engage in purely cosmetic changes.
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
I also found at least one other place in libpq where it still does
fprintf(stderr). That should probably be fixed at the same time, right?
Yeah, we should be using the error message buffer if at all possible.
regards, tom lane
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
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 :-)AFAIR nobody got round to it because it hadn't seemed important.
Ok. I actually managed to provoke a GSSAPI error that got cut off at 256
characters in testing. Which is kind of amazing in itself, but...
(Question about backpatch remains)
I'd vote against backpatching. The appropriate fix for back branches
is probably just to reduce the strncpy and snprintf arguments to
INITIAL_EXPBUFFER_SIZE, ie, make the code do what that header comment
says it should do.
Right. See other mail as well.
Style point: in the places where you've chosen to pass the whole PGconn,
you should remove any separate arguments that are actually just PGconn
fields; eg for pg_krb5_sendauth it looks like sock and servicename are
now redundant. Otherwise there are risks of programmer confusion, and
maybe even wrong code generation, due to aliasing.It would be more consistent to pass PGconn around to all of these
functions instead of trying to have them have just partial views of it,
but I dunno if you want to engage in purely cosmetic changes.
I'll go ahead and do that now, while I'm whacking the code around.
//Magnus
Magnus Hagander <magnus@hagander.net> writes:
I never actually tested if it crashes on mingw, but looking some more at it
it really should - once one of these errors happen.
Hm. Much easier than that - the code is new in HEAD. 8.2 did
fprintf(stderr). And HEAD still does that in at least one case.
Anyway, I'll go ahead with the patch I wrote since it does Seem Nicer to
actually use the PQexpbuffer code there, and the patch was rather
trivial, but it's certainly not something to backpatch then...
It does look like there is a risk in 8.2 and before, though:
the fe-auth.c code has a lot of snprintf's with PQERRORMSG_LENGTH,
which should all be INITIAL_EXPBUFFER_SIZE according to that header
comment. snprintf typically doesn't write more than it has to,
but if there ever were a message exceeding INITIAL_EXPBUFFER_SIZE
we'd be at risk of a memory clobber. So that should be changed
as far back as it does that. Do you want to take care of it?
I can if you don't want to.
regards, tom lane
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
I never actually tested if it crashes on mingw, but looking some more at it
it really should - once one of these errors happen.Hm. Much easier than that - the code is new in HEAD. 8.2 did
fprintf(stderr). And HEAD still does that in at least one case.Anyway, I'll go ahead with the patch I wrote since it does Seem Nicer to
actually use the PQexpbuffer code there, and the patch was rather
trivial, but it's certainly not something to backpatch then...It does look like there is a risk in 8.2 and before, though:
the fe-auth.c code has a lot of snprintf's with PQERRORMSG_LENGTH,
which should all be INITIAL_EXPBUFFER_SIZE according to that header
comment. snprintf typically doesn't write more than it has to,
but if there ever were a message exceeding INITIAL_EXPBUFFER_SIZE
we'd be at risk of a memory clobber. So that should be changed
as far back as it does that. Do you want to take care of it?
I can if you don't want to.
Oh, didn't realize that one.
I can take a look at that as well, once I'm done with this one. Seems
easy enough - I'll leave you to focus on the more difficult stuff :-)
//Magnus