On 12/13/2011 05:45 PM, Alexander Shulgin wrote:
Before that, why don't also accept "psql://", "pgsql://", "postgre://"
and anything else? Or wait, aren't we adding to the soup again (or
rather putting the soup right into libpq?)
There are multiple URI samples within PostgreSQL drivers in the field,
here are two I know of what I believe to be a larger number of samples
that all match in this regard:
http://sequel.rubyforge.org/rdoc/files/doc/opening_databases_rdoc.html
http://www.rmunn.com/sqlalchemy-tutorial/tutorial.html
These two are using "postgres". One of the hopes in adding URI support
was to make it possible for the libpq spec to look similar to the ones
already floating around, so that they'd all converge. Using a different
prefix than the most popular ones have already adopted isn't a good way
to start that. Now, whenever the URI discussion wanders off into
copying the JDBC driver I wonder again why that's relevant. But making
the implementation look like what people have already deployed surely
is, particularly if there's no downside to doing that.
Initial quick review of your patch: you suggested this as the general form:
psql -d postgresql://user@pw:host:port/dbname?param1=value1¶m2=value2...
That's presumably supposed to be:
psql -d postgresql://user:pw@host:port/dbname?param1=value1¶m2=value2...
This variation worked here:
$ psql -d postgresql://gsmith@localhost:5432/gsmith
If we had to pick one URI prefix, it should be "postgres". But given
the general name dysfunction around this project, I can't see how anyone
would complain if we squat on "postgresql" too. Attached patch modifies
yours to prove we can trivially support both, in hopes of detonating
this argument before it rages on further. Tested like this:
$ psql -d postgres://gsmith@localhost:5432/gsmith
And that works too now. I doubt either of us like what I did to the
handoff between conninfo_uri_parse and conninfo_uri_parse_options to
achieve that, but this feature is still young.
After this bit of tinkering with the code, it feels to me like this
really wants a split() function to break out the two sides of a string
across a delimiter, eating it in the process. Adding the level of
paranoia I'd like around every bit of code I see that does that type of
operation right now would take a while. Refactoring in terms of split
and perhaps a few similarly higher-level string parsing operations,
targeted for this job, might make it easier to focus on fortifying those
library routines instead. For example, instead of the gunk I just added
that moves past either type of protocol prefix, I'd like to just say
"split(buf,"://",&left,&right) and then move on with processing the
right side.
I agree with your comment that we need to add some sort of regression
tests for this. Given how the parsing is done right now, we'd want to
come up with some interesting invalid strings too. Making sure this
fails gracefully (and not in a buffer overflow way) might even use
something like fuzz testing too. Around here we've just been building
some Python scripting to do that sort of thing, tests that aren't
practical to do with pg_regress. Probably be better from the project's
perspective if such things were in Perl instead; so far no one has ever
paid me enough to stomach writing non-trivial things in Perl. Perhaps
you are more diverse.
--
Greg Smith 2ndQuadrant US g...@2ndquadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 50f3f83..9c10abf 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** static const PQEnvironmentOption Environ
*** 282,287 ****
--- 282,291 ----
}
};
+ /* The recognized connection URI must start with one of the following designators: */
+ static const char uri_designator[] = "postgresql://";
+ static const char short_uri_designator[] = "postgres://";
+
static bool connectOptions1(PGconn *conn, const char *conninfo);
static bool connectOptions2(PGconn *conn);
*************** static PQconninfoOption *conninfo_parse(
*** 297,303 ****
static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
const char *const * values, PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
! static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
static void defaultNoticeProcessor(void *arg, const char *message);
--- 301,326 ----
static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
const char *const * values, PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
! static PQconninfoOption *conninfo_uri_parse(const char *uri,
! PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_options(PQconninfoOption *options,
! const char *uri, char *buf,
! PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_params(char *params,
! PQconninfoOption *connOptions,
! PQExpBuffer errorMessage);
! static char *conninfo_uri_decode(const char *str, PQExpBuffer errorMessage);
! static bool get_hexdigit(char digit, int *value);
! static const char *conninfo_getval(PQconninfoOption *connOptions,
! const char *keyword);
! static PQconninfoOption *conninfo_storeval(PQconninfoOption *connOptions,
! const char *keyword, const char *value,
! PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_store_uri_encoded_value(
! PQconninfoOption *connOptions,
! const char *keyword, const char *encoded_value,
! PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_find(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
static void defaultNoticeProcessor(void *arg, const char *message);
*************** PQconnectStart(const char *conninfo)
*** 580,586 ****
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
! char *tmp;
/*
* Move option values into conn structure
--- 603,609 ----
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
! const char *tmp;
/*
* Move option values into conn structure
*************** ldapServiceLookup(const char *purl, PQco
*** 3739,3745 ****
static int
parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
{
! char *service = conninfo_getval(options, "service");
char serviceFile[MAXPGPATH];
char *env;
bool group_found = false;
--- 3762,3768 ----
static int
parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
{
! const char *service = conninfo_getval(options, "service");
char serviceFile[MAXPGPATH];
char *env;
bool group_found = false;
*************** conninfo_parse(const char *conninfo, PQE
*** 4129,4161 ****
}
/*
! * Now we have the name and the value. Search for the param record.
! */
! for (option = options; option->keyword != NULL; option++)
! {
! if (strcmp(option->keyword, pname) == 0)
! break;
! }
! if (option->keyword == NULL)
! {
! printfPQExpBuffer(errorMessage,
! libpq_gettext("invalid connection option \"%s\"\n"),
! pname);
! PQconninfoFree(options);
! free(buf);
! return NULL;
! }
!
! /*
! * Store the value
*/
! if (option->val)
! free(option->val);
! option->val = strdup(pval);
! if (!option->val)
{
- printfPQExpBuffer(errorMessage,
- libpq_gettext("out of memory\n"));
PQconninfoFree(options);
free(buf);
return NULL;
--- 4152,4161 ----
}
/*
! * Now that we have the name and the value, store the record.
*/
! if (!conninfo_storeval(options, pname, pval, errorMessage, false))
{
PQconninfoFree(options);
free(buf);
return NULL;
*************** conninfo_array_parse(const char *const *
*** 4276,4283 ****
/* first find "dbname" if any */
if (strcmp(pname, "dbname") == 0)
{
/* next look for "=" in the value */
! if (pvalue && strchr(pvalue, '='))
{
/*
* Must be a conninfo string, so parse it, but do not use
--- 4276,4299 ----
/* first find "dbname" if any */
if (strcmp(pname, "dbname") == 0)
{
+ /*
+ * Look for URI prefix. This has to come before the check for "=",
+ * since URI might as well contain "=" if extra parameters are
+ * given.
+ */
+ if (pvalue && (
+ (strncmp(pvalue, uri_designator,
+ sizeof(uri_designator) - 1) == 0) ||
+ (strncmp(pvalue, short_uri_designator,
+ sizeof(short_uri_designator) - 1) == 0)
+ ))
+ {
+ str_options = conninfo_uri_parse(pvalue, errorMessage);
+ if (str_options == NULL)
+ return NULL;
+ }
/* next look for "=" in the value */
! else if (pvalue && strchr(pvalue, '='))
{
/*
* Must be a conninfo string, so parse it, but do not use
*************** conninfo_array_parse(const char *const *
*** 4452,4467 ****
return options;
}
static char *
conninfo_getval(PQconninfoOption *connOptions,
const char *keyword)
{
PQconninfoOption *option;
for (option = connOptions; option->keyword != NULL; option++)
{
if (strcmp(option->keyword, keyword) == 0)
! return option->val;
}
return NULL;
--- 4468,4970 ----
return options;
}
+ /*
+ * Connection URI parser routine
+ *
+ * If successful, a malloc'd PQconninfoOption array is returned.
+ * If not successful, NULL is returned and an error message is
+ * left in errorMessage.
+ *
+ * This is only a wrapper for conninfo_uri_parse_options, which does all of the
+ * heavy lifting, while this function handles proper (de-)allocation of memory
+ * buffers.
+ */
+ static PQconninfoOption *
+ conninfo_uri_parse(const char *uri, PQExpBuffer errorMessage)
+ {
+ PQconninfoOption *options;
+ char *buf;
+
+ resetPQExpBuffer(errorMessage);
+
+ /* Make a working copy of PQconninfoOptions */
+ options = malloc(sizeof(PQconninfoOptions));
+ if (options == NULL)
+ {
+ printfPQExpBuffer(errorMessage, libpq_gettext("out of memory\n"));
+ return NULL;
+ }
+ memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+
+ /* Make a writable copy of the URI buffer */
+ buf = strdup(uri);
+ if (buf == NULL)
+ {
+ printfPQExpBuffer(errorMessage, libpq_gettext("out of memory\n"));
+ free(options);
+ return NULL;
+ }
+
+ if (!conninfo_uri_parse_options(options, uri, buf, errorMessage))
+ {
+ free(options);
+ options = NULL;
+ }
+
+ free(buf);
+ return options;
+ }
+
+ /*
+ * Connection URI parser: actual parser routine
+ *
+ * If successful, returns true while the options array is filled with parsed
+ * options from the passed URI
+ * If not successful, returns false and fills errorMessage accordingly.
+ *
+ * Parses the connection URI string in 'buf' (while destructively modifying
+ * it,) according to the URI syntax below. The 'uri' parameter is only used
+ * for error reporting and 'buf' should initially contain a writable copy of
+ * 'uri'.
+ *
+ * The general form for connection URI is the following:
+ *
+ * postgresql://user:pw@host:port/database
+ *
+ * To specify a IPv6 host address, enclose the address in square brackets:
+ *
+ * postgresql://[::1]/database
+ *
+ * Connection parameters may follow the base URI using this syntax:
+ *
+ * postgresql://host/database?param1=value1¶m2=value2&...
+ *
+ * In each of these cases, "postgres" is accepted as a substitute for
+ * "postgresql" at the beginning of the URI.
+ *
+ */
+ static bool
+ conninfo_uri_parse_options(PQconninfoOption *options, const char* uri,
+ char *buf, PQExpBuffer errorMessage)
+ {
+ /* Assume host address, unless seen otherwise */
+ const char *host;
+ const char *dbname;
+ bool has_port = false;
+ bool has_params = false;
+ char *start = buf;
+ char *p;
+
+ /*
+ * Assume one of the valid URI prefixes is already verified by the caller,
+ * we just need to figure out which one.
+ */
+ if (strncmp(start, uri_designator,
+ sizeof(uri_designator) - 1) == 0)
+ start+=sizeof(uri_designator) - 1;
+ else
+ start+=sizeof(short_uri_designator) - 1;
+ p=start;
+
+ /* Look ahead for possible user credentials designator */
+ while (*p && *p != '@' && *p != '/')
+ ++p;
+ if (*p != '@')
+ {
+ /* Reset to start of URI and parse as hostname/addr instead */
+ p = start;
+ }
+ else
+ {
+ char *user = start;
+ bool has_password = false;
+
+ p = user;
+ while (*p != ':' && *p != '@')
+ ++p;
+ if (*p == ':')
+ has_password = true;
+
+ /* Cut off at end of user name */
+ *p = '\0';
+
+ if (!conninfo_store_uri_encoded_value(options, "user", user,
+ errorMessage, false))
+ return false;
+
+ if (has_password)
+ {
+ const char *password = p + 1;
+
+ while (*p != '@')
+ ++p;
+ *p = '\0';
+
+ if (!conninfo_store_uri_encoded_value(options, "password", password,
+ errorMessage, false))
+ return false;
+ }
+
+ /* Advance past end of parsed user name or password token */
+ ++p;
+ }
+
+ /* Look for IPv6 address */
+ if (*p == '[')
+ {
+ host = ++p;
+ while (*p && *p != ']')
+ ++p;
+ if (!*p)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("end of string reached when looking for matching ']' in IPv6 host address in URI: %s\n"),
+ uri);
+ return false;
+ }
+ if (p == host)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("IPv6 host address may not be empty in URI: %s\n"),
+ uri);
+ return false;
+ }
+
+ /* Cut off the bracket and advance */
+ *(p++) = '\0';
+
+ /* The address must be followed by a port specifier or a slash */
+ if (*p != ':' && *p != '/')
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("unexpected '%c' at position %td in URI (expecting ':' or '/'): %s\n"),
+ *p, p - buf + 1, uri);
+ return false;
+ }
+ }
+ else /* no IPv6 address */
+ {
+ host = p;
+ /*
+ * Look for port specifier (colon) or end of host specifier (slash)
+ */
+ while (*p && *p != ':' && *p != '/')
+ ++p;
+ if (!*p)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("end of string reached when looking for hostname/address specifier in URI: %s\n"),
+ uri);
+ return false;
+ }
+ }
+
+ if (*p == ':')
+ has_port = true;
+ *p = '\0';
+
+ if (!conninfo_store_uri_encoded_value(options, "host", host, errorMessage,
+ false))
+ return false;
+
+ if (has_port)
+ {
+ const char *port = ++p; /* advance past host terminator */
+
+ while (*p && *p != '/')
+ ++p;
+ if (!*p)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("end of string reached when looking for port specifier in URI: %s\n"),
+ uri);
+ return false;
+ }
+ *p = '\0';
+
+ if (!conninfo_store_uri_encoded_value(options, "port", port,
+ errorMessage, false))
+ return false;
+ }
+
+ /* `p' must be pointing at the slash at this time, advance */
+ dbname = ++p;
+ while (*p && *p != '?')
+ ++p;
+
+ /* Check for extra parameters */
+ if (*p == '?')
+ has_params = true;
+ *p = '\0';
+
+ if (!conninfo_store_uri_encoded_value(options, "dbname", dbname,
+ errorMessage, false))
+ return false;
+
+ if (has_params)
+ {
+ ++p; /* advance past dbname terminator */
+
+ if (!conninfo_uri_parse_params(p, options, errorMessage))
+ return false;
+ }
+
+ return true;
+ }
+
+ /*
+ * Connection URI parameters parser routine
+ *
+ * If successful, returns true while connOptions is filled with parsed
+ * parameters.
+ * If not successful, returns false and fills errorMessage accordingly.
+ *
+ * Destructively modifies 'params' buffer.
+ */
+ static bool
+ conninfo_uri_parse_params(char *params,
+ PQconninfoOption *connOptions,
+ PQExpBuffer errorMessage)
+ {
+ char *token;
+ char *savep;
+
+ while ((token = strtok_r(params, "&", &savep)))
+ {
+ const char *keyword;
+ const char *value;
+ char *p = strchr(token, '=');
+
+ if (p == NULL)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("missing key/value separator '=' in URI query parameter: %s\n"),
+ token);
+ return false;
+ }
+
+ /* Cut off keyword and advance to value */
+ *(p++) = '\0';
+
+ keyword = token;
+ value = p;
+
+ /* Special keyword handling for improved JDBC compatibility */
+ if (strcmp(keyword, "ssl") == 0 &&
+ strcmp(value, "true") == 0)
+ {
+ keyword = "sslmode";
+ value = "require";
+ }
+
+ /*
+ * Store the value if corresponding option was found -- ignore
+ * otherwise.
+ *
+ * In theory, the keyword might be also percent-encoded, but hardly
+ * that's ever needed by anyone.
+ */
+ conninfo_store_uri_encoded_value(connOptions, keyword, value,
+ errorMessage, true);
+
+ params = NULL; /* proceed to the next token with strtok_r */
+ }
+
+ return true;
+ }
+
+ /*
+ * Connection URI decoder routine
+ *
+ * If successful, returns the malloc'd decoded string.
+ * If not successful, returns NULL and fills errorMessage accordingly.
+ *
+ * The string is decoded by replacing any percent-encoded tokens with
+ * corresponding characters, while preserving any non-encoded characters. A
+ * percent-encoded token is a character triplet: a percent sign, followed by a
+ * pair of hexadecimal digits (0-9A-F), where lower- and upper-case letters are
+ * treated identically.
+ */
static char *
+ conninfo_uri_decode(const char *str, PQExpBuffer errorMessage)
+ {
+ char *buf = malloc(strlen(str) + 1);
+ char *p = buf;
+ const char *q = str;
+
+ if (buf == NULL)
+ {
+ printfPQExpBuffer(errorMessage, libpq_gettext("out of memory\n"));
+ return NULL;
+ }
+
+ for (;;)
+ {
+ if (*q != '%')
+ {
+ /* copy and check for NUL terminator */
+ if (!(*(p++) = *(q++)))
+ break;
+ }
+ else
+ {
+ int hi;
+ int lo;
+
+ ++q; /* skip the percent sign itself */
+
+ if (!(get_hexdigit(*q++, &hi) && get_hexdigit(*q++, &lo)))
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("invalid percent-encoded token (a pair of hexadecimal digits expected after every '%%' symbol, use '%%25' to encode percent symbol itself): %s\n"),
+ str);
+ free(buf);
+ return NULL;
+ }
+
+ *(p++) = (hi << 4) | lo;
+ }
+ }
+
+ return buf;
+ }
+
+ /*
+ * Convert hexadecimal digit character to it's integer value.
+ *
+ * If successful, returns true and value is filled with digit's base 16 value.
+ * If not successful, returns false.
+ *
+ * Lower- and upper-case letters in the range A-F are treated identically.
+ */
+ static bool
+ get_hexdigit(char digit, int *value)
+ {
+ if ('0' <= digit && digit <= '9')
+ {
+ *value = digit - '0';
+ }
+ else if ('A' <= digit && digit <= 'F')
+ {
+ *value = digit - 'A' + 10;
+ }
+ else if ('a' <= digit && digit <= 'f')
+ {
+ *value = digit - 'a' + 10;
+ }
+ else
+ {
+ return false;
+ }
+
+ return true;
+ }
+
+ /*
+ * Find an option value corresponding to the keyword in the connOptions array.
+ *
+ * If successful, returns a pointer to the corresponding option's value.
+ * If not successful, returns NULL.
+ */
+ static const char *
conninfo_getval(PQconninfoOption *connOptions,
const char *keyword)
{
+ PQconninfoOption *option = conninfo_find(connOptions, keyword);
+
+ return option ? option->val : NULL;
+ }
+
+ /*
+ * Store a (new) value for an option corresponding to the keyword in
+ * connOptions array.
+ *
+ * If successful, returns a pointer to the corresponding PQconninfoOption,
+ * which value is replaced with a strdup'd copy of the passed value string.
+ * The existing value for the option is free'd before replacing, if any.
+ *
+ * If not successful, returns NULL and fills errorMessage accordingly.
+ */
+ static PQconninfoOption *
+ conninfo_storeval(PQconninfoOption *connOptions,
+ const char *keyword, const char *value,
+ PQExpBuffer errorMessage, bool ignoreMissing)
+ {
+ PQconninfoOption *option = conninfo_find(connOptions, keyword);
+ char *value_copy;
+
+ if (option == NULL)
+ {
+ if (!ignoreMissing)
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("invalid connection option \"%s\"\n"),
+ keyword);
+ return NULL;
+ }
+
+ value_copy = strdup(value);
+ if (value_copy == NULL)
+ {
+ printfPQExpBuffer(errorMessage, libpq_gettext("out of memory\n"));
+ return NULL;
+ }
+
+ if (option->val)
+ free(option->val);
+ option->val = value_copy;
+
+ return option;
+ }
+
+ /*
+ * Store a (new) possibly URI-encoded value for an option corresponding to the
+ * keyword in connOptions array.
+ *
+ * If successful, returns a pointer to the corresponding PQconninfoOption,
+ * which value is replaced with a URI-decoded copy of the passed value string.
+ * The existing value for the option is free'd before replacing, if any.
+ *
+ * If not successful, returns NULL and fills errorMessage accordingly. If the
+ * corresponding option was not found, but ignoreMissing is true: doesn't fill
+ * errorMessage.
+ *
+ * See also conninfo_storeval.
+ */
+ static PQconninfoOption *
+ conninfo_store_uri_encoded_value(PQconninfoOption *connOptions,
+ const char *keyword, const char *encoded_value,
+ PQExpBuffer errorMessage, bool ignoreMissing)
+ {
+ PQconninfoOption *option;
+ char *decoded_value = conninfo_uri_decode(encoded_value, errorMessage);
+
+ if (decoded_value == NULL)
+ return NULL;
+
+ option = conninfo_storeval(connOptions, keyword, decoded_value,
+ errorMessage, ignoreMissing);
+ free(decoded_value);
+
+ return option;
+ }
+
+ /*
+ * Find a PQconninfoOption option corresponding to the keyword in the
+ * connOptions array.
+ *
+ * If successful, returns a pointer to the corresponding PQconninfoOption
+ * structure.
+ * If not successful, returns NULL.
+ */
+ static PQconninfoOption *
+ conninfo_find(PQconninfoOption *connOptions, const char *keyword)
+ {
PQconninfoOption *option;
for (option = connOptions; option->keyword != NULL; option++)
{
if (strcmp(option->keyword, keyword) == 0)
! return option;
}
return NULL;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
