From 9a1d0af4ad2cbd419115b453d811c141b80d872b Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Tue, 22 Nov 2016 15:32:13 -0500 Subject: [PATCH] Code review for commit 274bb2b3857cc987cfa21d14775cae9b0dababa5. Avoid memory leak in conninfo_uri_parse_options. Use the current host rather than the comma-separated list of host names when the host name is needed for GSS, SSPI, or SSL authentication. Document the way connect_timeout interacts with multiple host specifications. Takayuki Tsunakawa --- doc/src/sgml/libpq.sgml | 4 ++++ src/interfaces/libpq/fe-auth.c | 12 +++++++----- src/interfaces/libpq/fe-connect.c | 9 +++++---- src/interfaces/libpq/fe-secure-openssl.c | 12 +++++++----- 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index d04dba7493..0f375bf5f2 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1009,6 +1009,10 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname Maximum wait for connection, in seconds (write as a decimal integer string). Zero or not specified means wait indefinitely. It is not recommended to use a timeout of less than 2 seconds. + This timeout applies separately to each connection attempt. + For example, if you specify two hosts and both of them are unreachable, + and connect_timeout is 5, the total time spent waiting for a + connection might be up to 10 seconds. diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 19171fb676..d861dc487b 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -170,8 +170,9 @@ pg_GSS_startup(PGconn *conn) min_stat; int maxlen; gss_buffer_desc temp_gbuf; + char *host = PQhost(conn); - if (!(conn->pghost && conn->pghost[0] != '\0')) + if (!(host && host[0] != '\0')) { printfPQExpBuffer(&conn->errorMessage, libpq_gettext("host name must be specified\n")); @@ -198,7 +199,7 @@ pg_GSS_startup(PGconn *conn) return STATUS_ERROR; } snprintf(temp_gbuf.value, maxlen, "%s@%s", - conn->krbsrvname, conn->pghost); + conn->krbsrvname, host); temp_gbuf.length = strlen(temp_gbuf.value); maj_stat = gss_import_name(&min_stat, &temp_gbuf, @@ -371,6 +372,7 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate) { SECURITY_STATUS r; TimeStamp expire; + char *host = PQhost(conn); conn->sspictx = NULL; @@ -406,19 +408,19 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate) * but not more complex. We can skip the @REALM part, because Windows will * fill that in for us automatically. */ - if (!(conn->pghost && conn->pghost[0] != '\0')) + if (!(host && host[0] != '\0')) { printfPQExpBuffer(&conn->errorMessage, libpq_gettext("host name must be specified\n")); return STATUS_ERROR; } - conn->sspitarget = malloc(strlen(conn->krbsrvname) + strlen(conn->pghost) + 2); + conn->sspitarget = malloc(strlen(conn->krbsrvname) + strlen(host) + 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); + sprintf(conn->sspitarget, "%s/%s", conn->krbsrvname, host); /* * Indicate that we're in SSPI authentication mode to make sure that diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index ae85db9dd5..3e9c45bc40 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -4931,7 +4931,7 @@ conninfo_uri_parse_options(PQconninfoOption *options, const char *uri, { int prefix_len; char *p; - char *buf; + char *buf = NULL; char *start; char prevchar = '\0'; char *user = NULL; @@ -4946,7 +4946,7 @@ conninfo_uri_parse_options(PQconninfoOption *options, const char *uri, { printfPQExpBuffer(errorMessage, libpq_gettext("out of memory\n")); - return false; + goto cleanup; } /* need a modifiable copy of the input URI */ @@ -4955,7 +4955,7 @@ conninfo_uri_parse_options(PQconninfoOption *options, const char *uri, { printfPQExpBuffer(errorMessage, libpq_gettext("out of memory\n")); - return false; + goto cleanup; } start = buf; @@ -5156,7 +5156,8 @@ conninfo_uri_parse_options(PQconninfoOption *options, const char *uri, cleanup: termPQExpBuffer(&hostbuf); termPQExpBuffer(&portbuf); - free(buf); + if (buf) + free(buf); return retval; } diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index f474c96f5f..7bdf92701a 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -483,6 +483,7 @@ verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *name_entry, char *name; const unsigned char *namedata; int result; + char *host = PQhost(conn); *store_name = NULL; @@ -528,12 +529,12 @@ verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *name_entry, return -1; } - if (pg_strcasecmp(name, conn->pghost) == 0) + if (pg_strcasecmp(name, host) == 0) { /* Exact name match */ result = 1; } - else if (wildcard_certificate_match(name, conn->pghost)) + else if (wildcard_certificate_match(name, host)) { /* Matched wildcard name */ result = 1; @@ -563,6 +564,7 @@ verify_peer_name_matches_certificate(PGconn *conn) STACK_OF(GENERAL_NAME) *peer_san; int i; int rc; + char *host = PQhost(conn); /* * If told not to verify the peer name, don't do it. Return true @@ -572,7 +574,7 @@ verify_peer_name_matches_certificate(PGconn *conn) return true; /* Check that we have a hostname to compare with. */ - if (!(conn->pghost && conn->pghost[0] != '\0')) + if (!(host && host[0] != '\0')) { printfPQExpBuffer(&conn->errorMessage, libpq_gettext("host name must be specified for a verified SSL connection\n")); @@ -670,13 +672,13 @@ verify_peer_name_matches_certificate(PGconn *conn) libpq_ngettext("server certificate for \"%s\" (and %d other name) does not match host name \"%s\"\n", "server certificate for \"%s\" (and %d other names) does not match host name \"%s\"\n", names_examined - 1), - first_name, names_examined - 1, conn->pghost); + first_name, names_examined - 1, host); } else if (names_examined == 1) { printfPQExpBuffer(&conn->errorMessage, libpq_gettext("server certificate for \"%s\" does not match host name \"%s\"\n"), - first_name, conn->pghost); + first_name, host); } else {