Thread: libpq: fix unlikely memory leak
The attached patch fixes a theoretical memory leak in libpq: if the
second malloc() fails due to OOM, the memory returned by the first
(successful) malloc() will be leaked.
Barring any objections I'll apply this tomorrow.
Per report from EnterpriseDB's Coverity analysis.
-Neil
Index: src/interfaces/libpq/fe-auth.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.102
diff -c -r1.102 fe-auth.c
*** src/interfaces/libpq/fe-auth.c 27 Jun 2005 02:04:26 -0000 1.102
--- src/interfaces/libpq/fe-auth.c 29 Jun 2005 03:09:01 -0000
***************
*** 407,434 ****
{
char *crypt_pwd2;
! if (!(crypt_pwd = malloc(MD5_PASSWD_LEN + 1)) ||
! !(crypt_pwd2 = malloc(MD5_PASSWD_LEN + 1)))
{
fprintf(stderr, libpq_gettext("out of memory\n"));
! return STATUS_ERROR;
}
if (!EncryptMD5(password, conn->pguser,
strlen(conn->pguser), crypt_pwd2))
! {
! free(crypt_pwd);
! free(crypt_pwd2);
! return STATUS_ERROR;
! }
if (!EncryptMD5(crypt_pwd2 + strlen("md5"), conn->md5Salt,
sizeof(conn->md5Salt), crypt_pwd))
! {
! free(crypt_pwd);
! free(crypt_pwd2);
! return STATUS_ERROR;
! }
free(crypt_pwd2);
break;
}
case AUTH_REQ_CRYPT:
{
--- 407,438 ----
{
char *crypt_pwd2;
! crypt_pwd = malloc(MD5_PASSWD_LEN + 1);
! crypt_pwd2 = malloc(MD5_PASSWD_LEN + 1);
!
! if (crypt_pwd == NULL || crypt_pwd2 == NULL)
{
fprintf(stderr, libpq_gettext("out of memory\n"));
! goto md5_error;
}
+
if (!EncryptMD5(password, conn->pguser,
strlen(conn->pguser), crypt_pwd2))
! goto md5_error;
!
if (!EncryptMD5(crypt_pwd2 + strlen("md5"), conn->md5Salt,
sizeof(conn->md5Salt), crypt_pwd))
! goto md5_error;
!
free(crypt_pwd2);
break;
+
+ md5_error:
+ if (crypt_pwd)
+ free(crypt_pwd);
+ if (crypt_pwd2)
+ free(crypt_pwd2);
+ return STATUS_ERROR;
}
case AUTH_REQ_CRYPT:
{
Neil Conway <neilc@samurai.com> writes:
> The attached patch fixes a theoretical memory leak in libpq: if the
> second malloc() fails due to OOM, the memory returned by the first
> (successful) malloc() will be leaked.
Since both allocations are only transient within this routine, there's
a simpler more effective solution, which is to only do one malloc in
the first place:
char *crypt_pwd2;
/* need enough space for 2 MD5 hashes */
if (!(crypt_pwd = malloc(2 * (MD5_PASSWD_LEN + 1))))
{
fprintf(stderr, libpq_gettext("out of memory\n"));
return STATUS_ERROR;
}
crypt_pwd2 = crypt_pwd + (MD5_PASSWD_LEN + 1);
and drop the free(crypt_pwd2) calls.
regards, tom lane
Tom Lane wrote:
> Since both allocations are only transient within this routine, there's
> a simpler more effective solution, which is to only do one malloc in
> the first place
Yeah, true. Attached is a revised patch -- committed to HEAD.
-Neil
Index: src/interfaces/libpq/fe-auth.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.102
diff -c -r1.102 fe-auth.c
*** src/interfaces/libpq/fe-auth.c 27 Jun 2005 02:04:26 -0000 1.102
--- src/interfaces/libpq/fe-auth.c 30 Jun 2005 01:55:35 -0000
***************
*** 407,433 ****
{
char *crypt_pwd2;
! if (!(crypt_pwd = malloc(MD5_PASSWD_LEN + 1)) ||
! !(crypt_pwd2 = malloc(MD5_PASSWD_LEN + 1)))
{
fprintf(stderr, libpq_gettext("out of memory\n"));
return STATUS_ERROR;
}
if (!EncryptMD5(password, conn->pguser,
strlen(conn->pguser), crypt_pwd2))
{
free(crypt_pwd);
- free(crypt_pwd2);
return STATUS_ERROR;
}
if (!EncryptMD5(crypt_pwd2 + strlen("md5"), conn->md5Salt,
sizeof(conn->md5Salt), crypt_pwd))
{
free(crypt_pwd);
- free(crypt_pwd2);
return STATUS_ERROR;
}
- free(crypt_pwd2);
break;
}
case AUTH_REQ_CRYPT:
--- 407,433 ----
{
char *crypt_pwd2;
! /* Allocate enough space for two MD5 hashes */
! crypt_pwd = malloc(2 * (MD5_PASSWD_LEN + 1));
! if (!crypt_pwd)
{
fprintf(stderr, libpq_gettext("out of memory\n"));
return STATUS_ERROR;
}
+
+ crypt_pwd2 = crypt_pwd + MD5_PASSWD_LEN + 1;
if (!EncryptMD5(password, conn->pguser,
strlen(conn->pguser), crypt_pwd2))
{
free(crypt_pwd);
return STATUS_ERROR;
}
if (!EncryptMD5(crypt_pwd2 + strlen("md5"), conn->md5Salt,
sizeof(conn->md5Salt), crypt_pwd))
{
free(crypt_pwd);
return STATUS_ERROR;
}
break;
}
case AUTH_REQ_CRYPT: