[uri] Refactor URI parsing and formatting

Add support for parsing of URIs containing literal IPv6 addresses
(e.g. "http://[fe80::69ff:fe50:5845%25net0]/boot.ipxe").

Duplicate URIs by directly copying the relevant fields, rather than by
formatting and reparsing a URI string.  This relaxes the requirements
on the URI formatting code and allows it to focus on generating
human-readable URIs (e.g. by not escaping ':' characters within
literal IPv6 addresses).  As a side-effect, this allows relative URIs
containing parameter lists (e.g. "../boot.php##params") to function
as expected.

Add validity check for FTP paths to ensure that only printable
characters are accepted (since FTP is a human-readable line-based
protocol with no support for character escaping).

Construct TFTP next-server+filename URIs directly, rather than parsing
a constructed "tftp://..." string,

Add self-tests for URI functions.

Signed-off-by: Michael Brown <mcb30@ipxe.org>

src/arch/i386/image/multiboot.c

@@ -152,8 +152,7 @@ static physaddr_t multiboot_add_cmdline ( struct image *image ) {
 	size_t len;
 
 	/* Copy image URI to base memory buffer as start of command line */
-	len = ( unparse_uri ( buf, remaining, image->uri,
+	len = ( format_uri ( image->uri, buf, remaining ) + 1 /* NUL */ );
-			      URI_ALL ) + 1 /* NUL */ );
 	if ( len > remaining )
 		len = remaining;
 	mb_cmdline_offset += len;

src/core/uri.c

@@ -34,34 +34,184 @@ FILE_LICENCE ( GPL2_OR_LATER );
 #include <ipxe/params.h>
 #include <ipxe/uri.h>
 
+/**
+ * Decode URI field (in place)
+ *
+ * @v string		String
+ *
+ * URI decoding can never increase the length of a string; we can
+ * therefore safely decode in place.
+ */
+static void uri_decode ( char *string ) {
+	char *dest = string;
+	char hexbuf[3];
+	char *hexbuf_end;
+	char c;
+	char decoded;
+	unsigned int skip;
+
+	/* Copy string, decoding escaped characters as necessary */
+	do {
+		c = *(string++);
+		if ( c == '%' ) {
+			snprintf ( hexbuf, sizeof ( hexbuf ), "%s", string );
+			decoded = strtoul ( hexbuf, &hexbuf_end, 16 );
+			skip = ( hexbuf_end - hexbuf );
+			string += skip;
+			if ( skip )
+				c = decoded;
+		}
+		*(dest++) = c;
+	} while ( c );
+}
+
+/**
+ * Check if character should be escaped within a URI field
+ *
+ * @v c			Character
+ * @v field		URI field index
+ * @ret escaped		Character should be escaped
+ */
+static int uri_character_escaped ( char c, unsigned int field ) {
+
+	/* Non-printing characters and whitespace should always be
+	 * escaped, since they cannot sensibly be displayed as part of
+	 * a coherent URL string.  (This test also catches control
+	 * characters such as CR and LF, which could affect the
+	 * operation of line-based protocols such as HTTP.)
+	 *
+	 * We should also escape characters which would alter the
+	 * interpretation of the URL if not escaped, i.e. characters
+	 * which have significance to the URL parser.  We should not
+	 * blindly escape all such characters, because this would lead
+	 * to some very strange-looking URLs (e.g. if we were to
+	 * always escape '/' as "%2F" even within the URI path).
+	 *
+	 * We do not need to be perfect.  Our primary role is as a
+	 * consumer of URIs rather than a producer; the main situation
+	 * in which we produce a URI string is for display to a human
+	 * user, who can probably tolerate some variance from the
+	 * formal specification.  The only situation in which we
+	 * currently produce a URI string to be consumed by a computer
+	 * is when constructing an HTTP request URI, which contains
+	 * only the path and query fields.
+	 *
+	 * We can therefore sacrifice some correctness for the sake of
+	 * code size.  For example, colons within the URI host should
+	 * be escaped unless they form part of an IPv6 literal
+	 * address; doing this correctly would require the URI
+	 * formatter to be aware of whether or not the URI host
+	 * contained an IPv4 address, an IPv6 address, or a host name.
+	 * We choose to simplify and never escape colons within the
+	 * URI host field: in the event of a pathological hostname
+	 * containing colons, this could potentially produce a URI
+	 * string which could not be reparsed.
+	 *
+	 * After excluding non-printing characters, whitespace, and
+	 * '%', the full set of characters with significance to the
+	 * URL parser is "/#:@?".  We choose for each URI field which
+	 * of these require escaping in our use cases.
+	 */
+	static const char *escaped[URI_FIELDS] = {
+		/* Scheme: escape everything */
+		[URI_SCHEME]	= "/#:@?",
+		/* Opaque part: escape characters which would affect
+		 * the reparsing of the URI, allowing everything else
+		 * (e.g. ':', which will appear in iSCSI URIs).
+		 */
+		[URI_OPAQUE]	= "/#",
+		/* User name: escape everything */
+		[URI_USER]	= "/#:@?",
+		/* Password: escape everything */
+		[URI_PASSWORD]	= "/#:@?",
+		/* Host name: escape everything except ':', which may
+		 * appear as part of an IPv6 literal address.
+		 */
+		[URI_HOST]	= "/#@?",
+		/* Port number: escape everything */
+		[URI_PORT]	= "/#:@?",
+		/* Path: escape everything except '/', which usually
+		 * appears within paths.
+		 */
+		[URI_PATH]	= "#:@?",
+		/* Query: escape everything except '/', which
+		 * sometimes appears within queries.
+		 */
+		[URI_QUERY]	= "#:@?",
+		/* Fragment: escape everything */
+		[URI_FRAGMENT]	= "/#:@?",
+	};
+
+	return ( /* Always escape non-printing characters and whitespace */
+		 ( ! isprint ( c ) ) || ( c == ' ' ) ||
+		 /* Always escape '%' */
+		 ( c == '%' ) ||
+		 /* Escape field-specific characters */
+		 strchr ( escaped[field], c ) );
+}
+
+/**
+ * Encode URI field
+ *
+ * @v uri		URI
+ * @v field		URI field index
+ * @v buf		Buffer to contain encoded string
+ * @v len		Length of buffer
+ * @ret len		Length of encoded string (excluding NUL)
+ */
+size_t uri_encode ( const char *string, unsigned int field,
+		    char *buf, ssize_t len ) {
+	ssize_t remaining = len;
+	size_t used;
+	char c;
+
+	/* Ensure encoded string is NUL-terminated even if empty */
+	if ( len > 0 )
+		buf[0] = '\0';
+
+	/* Copy string, escaping as necessary */
+	while ( ( c = *(string++) ) ) {
+		if ( uri_character_escaped ( c, field ) ) {
+			used = ssnprintf ( buf, remaining, "%%%02X", c );
+		} else {
+			used = ssnprintf ( buf, remaining, "%c", c );
+		}
+		buf += used;
+		remaining -= used;
+	}
+
+	return ( len - remaining );
+}
+
 /**
  * Dump URI for debugging
  *
  * @v uri		URI
  */
-static void dump_uri ( struct uri *uri ) {
+static void uri_dump ( const struct uri *uri ) {
+
 	if ( ! uri )
 		return;
 	if ( uri->scheme )
-		DBG ( " scheme \"%s\"", uri->scheme );
+		DBGC ( uri, " scheme \"%s\"", uri->scheme );
 	if ( uri->opaque )
-		DBG ( " opaque \"%s\"", uri->opaque );
+		DBGC ( uri, " opaque \"%s\"", uri->opaque );
 	if ( uri->user )
-		DBG ( " user \"%s\"", uri->user );
+		DBGC ( uri, " user \"%s\"", uri->user );
 	if ( uri->password )
-		DBG ( " password \"%s\"", uri->password );
+		DBGC ( uri, " password \"%s\"", uri->password );
 	if ( uri->host )
-		DBG ( " host \"%s\"", uri->host );
+		DBGC ( uri, " host \"%s\"", uri->host );
 	if ( uri->port )
-		DBG ( " port \"%s\"", uri->port );
+		DBGC ( uri, " port \"%s\"", uri->port );
 	if ( uri->path )
-		DBG ( " path \"%s\"", uri->path );
+		DBGC ( uri, " path \"%s\"", uri->path );
 	if ( uri->query )
-		DBG ( " query \"%s\"", uri->query );
+		DBGC ( uri, " query \"%s\"", uri->query );
 	if ( uri->fragment )
-		DBG ( " fragment \"%s\"", uri->fragment );
+		DBGC ( uri, " fragment \"%s\"", uri->fragment );
 	if ( uri->params )
-		DBG ( " params \"%s\"", uri->params->name );
+		DBGC ( uri, " params \"%s\"", uri->params->name );
 }
 
 /**
@@ -69,7 +219,7 @@ static void dump_uri ( struct uri *uri ) {
  *
  * @v refcnt		Reference count
  */
-static void free_uri ( struct refcnt *refcnt ) {
+static void uri_free ( struct refcnt *refcnt ) {
 	struct uri *uri = container_of ( refcnt, struct uri, refcnt );
 
 	params_put ( uri->params );
@@ -93,16 +243,16 @@ struct uri * parse_uri ( const char *uri_string ) {
 	char *tmp;
 	char *path;
 	char *authority;
-	int i;
 	size_t raw_len;
+	unsigned int field;
 
 	/* Allocate space for URI struct and a copy of the string */
 	raw_len = ( strlen ( uri_string ) + 1 /* NUL */ );
 	uri = zalloc ( sizeof ( *uri ) + raw_len );
 	if ( ! uri )
 		return NULL;
-	ref_init ( &uri->refcnt, free_uri );
+	ref_init ( &uri->refcnt, uri_free );
-	raw = ( ( ( char * ) uri ) + sizeof ( *uri ) );
+	raw = ( ( ( void * ) uri ) + sizeof ( *uri ) );
 
 	/* Copy in the raw string */
 	memcpy ( raw, uri_string, raw_len );
@@ -125,12 +275,8 @@ struct uri * parse_uri ( const char *uri_string ) {
 		uri->fragment = tmp;
 	}
 
-	/* Identify absolute/relative URI.  We ignore schemes that are
+	/* Identify absolute/relative URI */
-	 * apparently only a single character long, since otherwise we
+	if ( ( tmp = strchr ( raw, ':' ) ) ) {
-	 * misinterpret a DOS-style path name ("C:\path\to\file") as a
-	 * URI with scheme="C",opaque="\path\to\file".
-	 */
-	if ( ( tmp = strchr ( raw, ':' ) ) && ( tmp > ( raw + 1 ) ) ) {
 		/* Absolute URI: identify hierarchical/opaque */
 		uri->scheme = raw;
 		*(tmp++) = '\0';
@@ -159,6 +305,12 @@ struct uri * parse_uri ( const char *uri_string ) {
 		uri->query = tmp;
 	}
 
+	/* If we have no path remaining, then we're already finished
+	 * processing.
+	 */
+	if ( ! path[0] )
+		goto done;
+
 	/* Identify net/absolute/relative path */
 	if ( strncmp ( path, "//", 2 ) == 0 ) {
 		/* Net path.  If this is terminated by the first '/'
@@ -205,23 +357,22 @@ struct uri * parse_uri ( const char *uri_string ) {
 	}
 
 	/* Split host into host[:port] */
-	if ( ( tmp = strchr ( uri->host, ':' ) ) ) {
+	if ( ( uri->host[ strlen ( uri->host ) - 1 ] != ']' ) &&
+	     ( tmp = strrchr ( uri->host, ':' ) ) ) {
 		*(tmp++) = '\0';
 		uri->port = tmp;
 	}
 
-	/* Decode fields that should be decoded */
+	/* Decode fields in-place */
-	for ( i = URI_FIRST_FIELD; i <= URI_LAST_FIELD; i++ ) {
+	for ( field = 0 ; field < URI_FIELDS ; field++ ) {
-		const char *field = uri_get_field ( uri, i );
+		if ( uri_field ( uri, field ) )
-		if ( field && ( URI_ENCODED & ( 1 << i ) ) )
+			uri_decode ( ( char * ) uri_field ( uri, field ) );
-			uri_decode ( field, ( char * ) field,
-				     strlen ( field ) + 1 /* NUL */ );
 	}
 
  done:
-	DBG ( "URI \"%s\" split into", uri_string );
+	DBGC ( uri, "URI parsed \"%s\" to", uri_string );
-	dump_uri ( uri );
+	uri_dump ( uri );
-	DBG ( "\n" );
+	DBGC ( uri, "\n" );
 
 	return uri;
 }
@@ -233,82 +384,137 @@ struct uri * parse_uri ( const char *uri_string ) {
  * @v default_port	Default port to use if none specified in URI
  * @ret port		Port
  */
-unsigned int uri_port ( struct uri *uri, unsigned int default_port ) {
+unsigned int uri_port ( const struct uri *uri, unsigned int default_port ) {
+
 	if ( ( ! uri ) || ( ! uri->port ) )
 		return default_port;
+
 	return ( strtoul ( uri->port, NULL, 0 ) );
 }
 
 /**
- * Unparse URI
+ * Format URI
  *
+ * @v uri		URI
  * @v buf		Buffer to fill with URI string
  * @v size		Size of buffer
- * @v uri		URI to write into buffer, or NULL
- * @v fields		Bitmask of fields to include in URI string, or URI_ALL
  * @ret len		Length of URI string
  */
-int unparse_uri ( char *buf, size_t size, struct uri *uri,
+size_t format_uri ( const struct uri *uri, char *buf, size_t len ) {
-		  unsigned int fields ) {
+	static const char prefixes[URI_FIELDS] = {
-	/* List of characters that typically go before certain fields */
+		[URI_OPAQUE] = ':',
-	static char separators[] = { /* scheme */ 0, /* opaque */ ':',
+		[URI_PASSWORD] = ':',
-				     /* user */ 0, /* password */ ':',
+		[URI_PORT] = ':',
-				     /* host */ '@', /* port */ ':',
+		[URI_PATH] = '/',
-				     /* path */ 0, /* query */ '?',
+		[URI_QUERY] = '?',
-				     /* fragment */ '#' };
+		[URI_FRAGMENT] = '#',
-	int used = 0;
+	};
-	int i;
+	char prefix;
-
+	size_t used = 0;
-	DBG ( "URI unparsing" );
+	unsigned int field;
-	dump_uri ( uri );
-	DBG ( "\n" );
 
 	/* Ensure buffer is NUL-terminated */
-	if ( size )
+	if ( len )
 		buf[0] = '\0';
 
 	/* Special-case NULL URI */
 	if ( ! uri )
 		return 0;
 
-	/* Iterate through requested fields */
+	/* Generate fields */
-	for ( i = URI_FIRST_FIELD; i <= URI_LAST_FIELD; i++ ) {
+	for ( field = 0 ; field < URI_FIELDS ; field++ ) {
-		const char *field = uri_get_field ( uri, i );
-		char sep = separators[i];
 
-		/* Ensure `fields' only contains bits for fields that exist */
+		/* Skip non-existent fields */
-		if ( ! field )
+		if ( ! uri_field ( uri, field ) )
-			fields &= ~( 1 << i );
+			continue;
 
-		/* Store this field if we were asked to */
+		/* Prefix this field, if applicable */
-		if ( fields & ( 1 << i ) ) {
+		prefix = prefixes[field];
-			/* Print :// if we're non-opaque and had a scheme */
+		if ( ( field == URI_HOST ) && ( uri->user != NULL ) )
-			if ( ( fields & URI_SCHEME_BIT ) &&
+			prefix = '@';
-			     ( i > URI_OPAQUE ) ) {
+		if ( ( field == URI_PATH ) && ( uri->path[0] == '/' ) )
-				used += ssnprintf ( buf + used, size - used,
+			prefix = '\0';
+		if ( prefix ) {
+			used += ssnprintf ( ( buf + used ), ( len - used ),
+					    "%c", prefix );
+		}
+
+		/* Encode this field */
+		used += uri_encode ( uri_field ( uri, field ), field,
+				     ( buf + used ), ( len - used ) );
+
+		/* Suffix this field, if applicable */
+		if ( ( field == URI_SCHEME ) && ( ! uri->opaque ) ) {
+			used += ssnprintf ( ( buf + used ), ( len - used ),
 					    "://" );
-				/* Only print :// once */
+		}
-				fields &= ~URI_SCHEME_BIT;
 	}
 
-			/* Only print separator if an earlier field exists */
+	if ( len ) {
-			if ( sep && ( fields & ( ( 1 << i ) - 1 ) ) )
+		DBGC ( uri, "URI formatted" );
-				used += ssnprintf ( buf + used, size - used,
+		uri_dump ( uri );
-						    "%c", sep );
+		DBGC ( uri, " to \"%s%s\"\n", buf,
-
+		       ( ( used > len ) ? "<TRUNCATED>" : "" ) );
-			/* Print contents of field, possibly encoded */
-			if ( URI_ENCODED & ( 1 << i ) )
-				used += uri_encode ( field, buf + used,
-						     size - used, i );
-			else
-				used += ssnprintf ( buf + used, size - used,
-						    "%s", field );
-		}
 	}
 
 	return used;
 }
 
+/**
+ * Format URI
+ *
+ * @v uri		URI
+ * @ret string		URI string, or NULL on failure
+ *
+ * The caller is responsible for eventually freeing the allocated
+ * memory.
+ */
+char * format_uri_alloc ( const struct uri *uri ) {
+	size_t len;
+	char *string;
+
+	len = ( format_uri ( uri, NULL, 0 ) + 1 /* NUL */ );
+	string = malloc ( len );
+	if ( string )
+		format_uri ( uri, string, len );
+	return string;
+}
+
+/**
+ * Copy URI fields
+ *
+ * @v src		Source URI
+ * @v dest		Destination URI, or NULL to calculate length
+ * @ret len		Length of raw URI
+ */
+static size_t uri_copy_fields ( const struct uri *src, struct uri *dest ) {
+	size_t len = sizeof ( *dest );
+	char *out = ( ( void * ) dest + len );
+	unsigned int field;
+	size_t field_len;
+
+	/* Copy existent fields */
+	for ( field = 0 ; field < URI_FIELDS ; field++ ) {
+
+		/* Skip non-existent fields */
+		if ( ! uri_field ( src, field ) )
+			continue;
+
+		/* Calculate field length */
+		field_len = ( strlen ( uri_field ( src, field ) )
+			      + 1 /* NUL */ );
+		len += field_len;
+
+		/* Copy field, if applicable */
+		if ( dest ) {
+			memcpy ( out, uri_field ( src, field ), field_len );
+			uri_field ( dest, field ) = out;
+			out += field_len;
+		}
+	}
+	return len;
+}
+
 /**
  * Duplicate URI
  *
@@ -317,12 +523,28 @@ int unparse_uri ( char *buf, size_t size, struct uri *uri,
  *
  * Creates a modifiable copy of a URI.
  */
-struct uri * uri_dup ( struct uri *uri ) {
+struct uri * uri_dup ( const struct uri *uri ) {
-	size_t len = ( unparse_uri ( NULL, 0, uri, URI_ALL ) + 1 );
+	struct uri *dup;
-	char buf[len];
+	size_t len;
 
-	unparse_uri ( buf, len, uri, URI_ALL );
+	/* Allocate new URI */
-	return parse_uri ( buf );
+	len = uri_copy_fields ( uri, NULL );
+	dup = zalloc ( len );
+	if ( ! dup )
+		return NULL;
+	ref_init ( &dup->refcnt, uri_free );
+
+	/* Copy fields */
+	uri_copy_fields ( uri, dup );
+
+	/* Copy parameters */
+	dup->params = params_get ( uri->params );
+
+	DBGC ( uri, "URI duplicated" );
+	uri_dump ( uri );
+	DBGC ( uri, "\n" );
+
+	return dup;
 }
 
 /**
@@ -398,7 +620,7 @@ char * resolve_path ( const char *base_path,
  * relative URI (e.g. "../initrds/initrd.gz") and produces a new URI
  * (e.g. "http://ipxe.org/initrds/initrd.gz").
  */
-struct uri * resolve_uri ( struct uri *base_uri,
+struct uri * resolve_uri ( const struct uri *base_uri,
 			   struct uri *relative_uri ) {
 	struct uri tmp_uri;
 	char *tmp_path = NULL;
@@ -417,11 +639,16 @@ struct uri * resolve_uri ( struct uri *base_uri,
 		tmp_uri.path = tmp_path;
 		tmp_uri.query = relative_uri->query;
 		tmp_uri.fragment = relative_uri->fragment;
+		tmp_uri.params = relative_uri->params;
 	} else if ( relative_uri->query ) {
 		tmp_uri.query = relative_uri->query;
 		tmp_uri.fragment = relative_uri->fragment;
+		tmp_uri.params = relative_uri->params;
 	} else if ( relative_uri->fragment ) {
 		tmp_uri.fragment = relative_uri->fragment;
+		tmp_uri.params = relative_uri->params;
+	} else if ( relative_uri->params ) {
+		tmp_uri.params = relative_uri->params;
 	}
 
 	/* Create demangled URI */
@@ -431,100 +658,23 @@ struct uri * resolve_uri ( struct uri *base_uri,
 }
 
 /**
- * Test for unreserved URI characters
+ * Construct TFTP URI from next-server and filename
  *
- * @v c			Character to test
+ * @v next_server	Next-server address
- * @v field		Field of URI in which character lies
+ * @v filename		Filename
- * @ret is_unreserved	Character is an unreserved character
+ * @ret uri		URI, or NULL on failure
+ *
+ * TFTP filenames specified via the DHCP next-server field often
+ * contain characters such as ':' or '#' which would confuse the
+ * generic URI parser.  We provide a mechanism for directly
+ * constructing a TFTP URI from the next-server and filename.
  */
-static int is_unreserved_uri_char ( int c, int field ) {
+struct uri * tftp_uri ( struct in_addr next_server, const char *filename ) {
-	/* According to RFC3986, the unreserved character set is
+	struct uri uri;
-	 *
-	 * A-Z a-z 0-9 - _ . ~
-	 *
-	 * but we also pass & ; = in queries, / in paths,
-	 * and everything in opaques
-	 */
-	int ok = ( isupper ( c ) || islower ( c ) || isdigit ( c ) ||
-		    ( c == '-' ) || ( c == '_' ) ||
-		    ( c == '.' ) || ( c == '~' ) );
 
-	if ( field == URI_QUERY )
+	memset ( &uri, 0, sizeof ( uri ) );
-		ok = ok || ( c == ';' ) || ( c == '&' ) || ( c == '=' );
+	uri.scheme = "tftp";
-
+	uri.host = inet_ntoa ( next_server );
-	if ( field == URI_PATH )
+	uri.path = filename;
-		ok = ok || ( c == '/' );
+	return uri_dup ( &uri );
-
-	if ( field == URI_OPAQUE )
-		ok = 1;
-
-	return ok;
-}
-
-/**
- * URI-encode string
- *
- * @v raw_string	String to be URI-encoded
- * @v buf		Buffer to contain encoded string
- * @v len		Length of buffer
- * @v field		Field of URI in which string lies
- * @ret len		Length of encoded string (excluding NUL)
- */
-size_t uri_encode ( const char *raw_string, char *buf, ssize_t len,
-		    int field ) {
-	ssize_t remaining = len;
-	size_t used;
-	unsigned char c;
-
-	if ( len > 0 )
-		buf[0] = '\0';
-
-	while ( ( c = *(raw_string++) ) ) {
-		if ( is_unreserved_uri_char ( c, field ) ) {
-			used = ssnprintf ( buf, remaining, "%c", c );
-		} else {
-			used = ssnprintf ( buf, remaining, "%%%02X", c );
-		}
-		buf += used;
-		remaining -= used;
-	}
-
-	return ( len - remaining );
-}
-
-/**
- * Decode URI-encoded string
- *
- * @v encoded_string	URI-encoded string
- * @v buf		Buffer to contain decoded string
- * @v len		Length of buffer
- * @ret len		Length of decoded string (excluding NUL)
- *
- * This function may be used in-place, with @a buf the same as
- * @a encoded_string.
- */
-size_t uri_decode ( const char *encoded_string, char *buf, ssize_t len ) {
-	ssize_t remaining;
-	char hexbuf[3];
-	char *hexbuf_end;
-	unsigned char c;
-
-	for ( remaining = len; *encoded_string; remaining-- ) {
-		if ( *encoded_string == '%' ) {
-			encoded_string++;
-			snprintf ( hexbuf, sizeof ( hexbuf ), "%s",
-				   encoded_string );
-			c = strtoul ( hexbuf, &hexbuf_end, 16 );
-			encoded_string += ( hexbuf_end - hexbuf );
-		} else {
-			c = *(encoded_string++);
-		}
-		if ( remaining > 1 )
-			*buf++ = c;
-	}
-
-	if ( len )
-		*buf = 0;
-
-	return ( len - remaining );
 }

src/crypto/ocsp.c

@@ -206,11 +206,12 @@ static int ocsp_request ( struct ocsp_check *ocsp ) {
  * @ret rc		Return status code
  */
 static int ocsp_uri_string ( struct ocsp_check *ocsp ) {
+	struct uri path_uri;
 	char *base_uri_string;
-	char *base64_request;
+	char *path_base64_string;
-	size_t base64_request_len;
+	char *path_uri_string;
-	size_t uri_string_len;
+	size_t path_len;
-	size_t prefix_len;
+	int len;
 	int rc;
 
 	/* Sanity check */
@@ -222,44 +223,43 @@ static int ocsp_uri_string ( struct ocsp_check *ocsp ) {
 		goto err_no_uri;
 	}
 
-	/* Base64-encode the request */
+	/* Base64-encode the request as the URI path */
-	base64_request_len = ( base64_encoded_len ( ocsp->request.builder.len )
+	path_len = ( base64_encoded_len ( ocsp->request.builder.len )
 		     + 1 /* NUL */ );
-	base64_request = malloc ( base64_request_len );
+	path_base64_string = malloc ( path_len );
-	if ( ! base64_request ) {
+	if ( ! path_base64_string ) {
 		rc = -ENOMEM;
-		goto err_alloc_base64;
+		goto err_path_base64;
 	}
 	base64_encode ( ocsp->request.builder.data, ocsp->request.builder.len,
-			base64_request );
+			path_base64_string );
 
-	/* Allocate URI string */
+	/* URI-encode the Base64-encoded request */
-	uri_string_len = ( strlen ( base_uri_string ) + 1 /* "/" */ +
+	memset ( &path_uri, 0, sizeof ( path_uri ) );
-			   uri_encode ( base64_request, NULL, 0, URI_FRAGMENT )
+	path_uri.path = path_base64_string;
-			   + 1 /* NUL */ );
+	path_uri_string = format_uri_alloc ( &path_uri );
-	ocsp->uri_string = malloc ( uri_string_len );
+	if ( ! path_uri_string ) {
-	if ( ! ocsp->uri_string ) {
 		rc = -ENOMEM;
-		goto err_alloc_uri;
+		goto err_path_uri;
 	}
 
 	/* Construct URI string */
-	prefix_len = snprintf ( ocsp->uri_string, uri_string_len,
+	if ( ( len = asprintf ( &ocsp->uri_string, "%s/%s", base_uri_string,
-				"%s/", base_uri_string );
+				path_uri_string ) ) < 0 ) {
-	uri_encode ( base64_request, ( ocsp->uri_string + prefix_len ),
+		rc = len;
-		     ( uri_string_len - prefix_len ), URI_FRAGMENT );
+		goto err_ocsp_uri;
+	}
 	DBGC2 ( ocsp, "OCSP %p \"%s\" URI is %s\n",
 		ocsp, ocsp->cert->subject.name, ocsp->uri_string );
 
-	/* Free base64-encoded request */
+	/* Success */
-	free ( base64_request );
+	rc = 0;
-	base64_request = NULL;
 
-	return 0;
+ err_ocsp_uri:
-
+	free ( path_uri_string );
- err_alloc_uri:
+ err_path_uri:
-	free ( base64_request );
+	free ( path_base64_string );
- err_alloc_base64:
+ err_path_base64:
  err_no_uri:
 	return rc;
 }

src/include/ipxe/uri.h

@@ -12,6 +12,7 @@ FILE_LICENCE ( GPL2_OR_LATER );
 #include <stddef.h>
 #include <stdlib.h>
 #include <ipxe/refcnt.h>
+#include <ipxe/in.h>
 
 struct parameters;
 
@@ -71,38 +72,39 @@ struct uri {
 	struct parameters *params;
 } __attribute__ (( packed ));
 
-/** A field in a URI
+/**
+ * Access URI field
  *
- * The order of the indices in this enumeration must match the order
+ * @v uri		URI
- * of the fields in the URI structure.
+ * @v field		URI field index
+ * @ret field		URI field (as an lvalue)
  */
-enum {
+#define uri_field( uri, field ) (&uri->scheme)[field]
-	URI_SCHEME = 0,		URI_SCHEME_BIT = ( 1 << URI_SCHEME ),
-	URI_OPAQUE = 1,		URI_OPAQUE_BIT = ( 1 << URI_OPAQUE ),
-	URI_USER = 2,		URI_USER_BIT = ( 1 << URI_USER ),
-	URI_PASSWORD = 3,	URI_PASSWORD_BIT = ( 1 << URI_PASSWORD ),
-	URI_HOST = 4,		URI_HOST_BIT = ( 1 << URI_HOST ),
-	URI_PORT = 5,		URI_PORT_BIT = ( 1 << URI_PORT ),
-	URI_PATH = 6,		URI_PATH_BIT = ( 1 << URI_PATH ),
-	URI_QUERY = 7,		URI_QUERY_BIT = ( 1 << URI_QUERY ),
-	URI_FRAGMENT = 8,	URI_FRAGMENT_BIT = ( 1 << URI_FRAGMENT ),
 
-	URI_FIRST_FIELD = URI_SCHEME,
+/**
-	URI_LAST_FIELD = URI_FRAGMENT,
+ * Calculate index of a URI field
+ *
+ * @v name		URI field name
+ * @ret field		URI field index
+ */
+#define URI_FIELD( name )						\
+	( ( offsetof ( struct uri, name ) -				\
+	    offsetof ( struct uri, scheme ) ) / sizeof ( void * ) )
+
+/** URI fields */
+enum uri_fields {
+	URI_SCHEME = URI_FIELD ( scheme ),
+	URI_OPAQUE = URI_FIELD ( opaque ),
+	URI_USER = URI_FIELD ( user ),
+	URI_PASSWORD = URI_FIELD ( password ),
+	URI_HOST = URI_FIELD ( host ),
+	URI_PORT = URI_FIELD ( port ),
+	URI_PATH = URI_FIELD ( path ),
+	URI_QUERY = URI_FIELD ( query ),
+	URI_FRAGMENT = URI_FIELD ( fragment ),
+	URI_FIELDS
 };
 
-/** Extract field from URI */
-#define uri_get_field( uri, field )	(&uri->scheme)[field]
-
-/** All URI fields */
-#define URI_ALL		( URI_SCHEME_BIT | URI_OPAQUE_BIT | URI_USER_BIT | \
-			  URI_PASSWORD_BIT | URI_HOST_BIT | URI_PORT_BIT | \
-			  URI_PATH_BIT | URI_QUERY_BIT | URI_FRAGMENT_BIT )
-
-/** URI fields that should be decoded on storage */
-#define URI_ENCODED	( URI_USER_BIT | URI_PASSWORD_BIT | URI_HOST_BIT | \
-			  URI_PATH_BIT | URI_QUERY_BIT | URI_FRAGMENT_BIT )
-
 /**
  * URI is an absolute URI
  *
@@ -125,8 +127,8 @@ static inline int uri_is_absolute ( const struct uri *uri ) {
  */
 static inline int uri_has_opaque ( const struct uri *uri ) {
 	return ( uri->opaque && ( uri->opaque[0] != '\0' ) );
-
 }
+
 /**
  * URI has a path
  *
@@ -189,18 +191,20 @@ uri_put ( struct uri *uri ) {
 
 extern struct uri *cwuri;
 
+extern size_t uri_encode ( const char *string, unsigned int field,
+			   char *buf, ssize_t len );
 extern struct uri * parse_uri ( const char *uri_string );
-extern unsigned int uri_port ( struct uri *uri, unsigned int default_port );
+extern size_t format_uri ( const struct uri *uri, char *buf, size_t len );
-extern int unparse_uri ( char *buf, size_t size, struct uri *uri,
+extern char * format_uri_alloc ( const struct uri *uri );
-			 unsigned int fields );
+extern unsigned int uri_port ( const struct uri *uri,
-extern struct uri * uri_dup ( struct uri *uri );
+			       unsigned int default_port );
+extern struct uri * uri_dup ( const struct uri *uri );
 extern char * resolve_path ( const char *base_path,
 			     const char *relative_path );
-extern struct uri * resolve_uri ( struct uri *base_uri,
+extern struct uri * resolve_uri ( const struct uri *base_uri,
 				  struct uri *relative_uri );
+extern struct uri * tftp_uri ( struct in_addr next_server,
+			       const char *filename );
 extern void churi ( struct uri *uri );
-extern size_t uri_encode ( const char *raw_string, char *buf, ssize_t len,
-			   int field );
-extern size_t uri_decode ( const char *encoded_string, char *buf, ssize_t len );
 
 #endif /* _IPXE_URI_H */

src/net/tcp/ftp.c

@@ -23,6 +23,7 @@
 #include <string.h>
 #include <assert.h>
 #include <errno.h>
+#include <ctype.h>
 #include <byteswap.h>
 #include <ipxe/socket.h>
 #include <ipxe/tcpip.h>
@@ -459,6 +460,25 @@ static struct interface_descriptor ftp_xfer_desc =
  *
  */
 
+/**
+ * Check validity of FTP control channel string
+ *
+ * @v string		String
+ * @ret rc		Return status code
+ */
+static int ftp_check_string ( const char *string ) {
+	char c;
+
+	/* The FTP control channel is line-based.  Check for invalid
+	 * non-printable characters (e.g. newlines).
+	 */
+	while ( ( c = *(string++) ) ) {
+		if ( ! isprint ( c ) )
+			return -EINVAL;
+	}
+	return 0;
+}
+
 /**
  * Initiate an FTP connection
  *
@@ -472,10 +492,17 @@ static int ftp_open ( struct interface *xfer, struct uri *uri ) {
 	int rc;
 
 	/* Sanity checks */
-	if ( ! uri->path )
-		return -EINVAL;
 	if ( ! uri->host )
 		return -EINVAL;
+	if ( ! uri->path )
+		return -EINVAL;
+	if ( ( rc = ftp_check_string ( uri->path ) ) != 0 )
+		return rc;
+	if ( uri->user && ( ( rc = ftp_check_string ( uri->user ) ) != 0 ) )
+		return rc;
+	if ( uri->password &&
+	     ( ( rc = ftp_check_string ( uri->password ) ) != 0 ) )
+		return rc;
 
 	/* Allocate and populate structure */
 	ftp = zalloc ( sizeof ( *ftp ) );

src/net/tcp/httpcore.c

@@ -958,8 +958,8 @@ static void http_socket_close ( struct http_request *http, int rc ) {
  */
 static char * http_basic_auth ( struct http_request *http ) {
 	const char *user = http->uri->user;
-	const char *password =
+	const char *password = ( http->uri->password ?
-		( http->uri->password ? http->uri->password : "" );
+				 http->uri->password : "" );
 	size_t user_pw_len =
 		( strlen ( user ) + 1 /* ":" */ + strlen ( password ) );
 	char user_pw[ user_pw_len + 1 /* NUL */ ];
@@ -1000,8 +1000,8 @@ static char * http_basic_auth ( struct http_request *http ) {
 static char * http_digest_auth ( struct http_request *http,
 				 const char *method, const char *uri ) {
 	const char *user = http->uri->user;
-	const char *password =
+	const char *password = ( http->uri->password ?
-		( http->uri->password ? http->uri->password : "" );
+				 http->uri->password : "" );
 	const char *realm = http->auth_realm;
 	const char *nonce = http->auth_nonce;
 	const char *opaque = http->auth_opaque;
@@ -1088,7 +1088,7 @@ static size_t http_post_params ( struct http_request *http,
 		}
 
 		/* URI-encode the key */
-		frag_len = uri_encode ( param->key, buf, remaining, 0 );
+		frag_len = uri_encode ( param->key, 0, buf, remaining );
 		buf += frag_len;
 		len += frag_len;
 		remaining -= frag_len;
@@ -1101,7 +1101,7 @@ static size_t http_post_params ( struct http_request *http,
 		remaining--;
 
 		/* URI-encode the value */
-		frag_len = uri_encode ( param->value, buf, remaining, 0 );
+		frag_len = uri_encode ( param->value, 0, buf, remaining );
 		buf += frag_len;
 		len += frag_len;
 		remaining -= frag_len;
@@ -1149,9 +1149,11 @@ static struct io_buffer * http_post ( struct http_request *http ) {
  */
 static void http_step ( struct http_request *http ) {
 	struct io_buffer *post;
-	size_t uri_len;
+	struct uri host_uri;
+	struct uri path_uri;
+	char *host_uri_string;
+	char *path_uri_string;
 	char *method;
-	char *uri;
 	char *range;
 	char *auth;
 	char *content;
@@ -1176,19 +1178,24 @@ static void http_step ( struct http_request *http ) {
 	method = ( ( http->flags & HTTP_HEAD_ONLY ) ? "HEAD" :
 		   ( http->uri->params ? "POST" : "GET" ) );
 
-	/* Construct path?query request */
+	/* Construct host URI */
-	uri_len = ( unparse_uri ( NULL, 0, http->uri,
+	memset ( &host_uri, 0, sizeof ( host_uri ) );
-				  URI_PATH_BIT | URI_QUERY_BIT )
+	host_uri.host = http->uri->host;
-		    + 1 /* possible "/" */ + 1 /* NUL */ );
+	host_uri.port = http->uri->port;
-	uri = malloc ( uri_len );
+	host_uri_string = format_uri_alloc ( &host_uri );
-	if ( ! uri ) {
+	if ( ! host_uri_string ) {
 		rc = -ENOMEM;
-		goto err_uri;
+		goto err_host_uri;
 	}
-	unparse_uri ( uri, uri_len, http->uri, URI_PATH_BIT | URI_QUERY_BIT );
+
-	if ( ! uri[0] ) {
+	/* Construct path URI */
-		uri[0] = '/';
+	memset ( &path_uri, 0, sizeof ( path_uri ) );
-		uri[1] = '\0';
+	path_uri.path = ( http->uri->path ? http->uri->path : "/" );
+	path_uri.query = http->uri->query;
+	path_uri_string = format_uri_alloc ( &path_uri );
+	if ( ! path_uri_string ) {
+		rc = -ENOMEM;
+		goto err_path_uri;
 	}
 
 	/* Calculate range request parameters if applicable */
@@ -1213,7 +1220,7 @@ static void http_step ( struct http_request *http ) {
 			goto err_auth;
 		}
 	} else if ( http->flags & HTTP_DIGEST_AUTH ) {
-		auth = http_digest_auth ( http, method, uri );
+		auth = http_digest_auth ( http, method, path_uri_string );
 		if ( ! auth ) {
 			rc = -ENOMEM;
 			goto err_auth;
@@ -1248,14 +1255,11 @@ static void http_step ( struct http_request *http ) {
 	if ( ( rc = xfer_printf ( &http->socket,
 				  "%s %s HTTP/1.1\r\n"
 				  "User-Agent: iPXE/%s\r\n"
-				  "Host: %s%s%s\r\n"
+				  "Host: %s\r\n"
 				  "%s%s%s%s"
 				  "\r\n",
-				  method, uri, product_version, http->uri->host,
+				  method, path_uri_string, product_version,
-				  ( http->uri->port ?
+				  host_uri_string,
-				    ":" : "" ),
-				  ( http->uri->port ?
-				    http->uri->port : "" ),
 				  ( ( http->flags & HTTP_CLIENT_KEEPALIVE ) ?
 				    "Connection: keep-alive\r\n" : "" ),
 				  ( range ? range : "" ),
@@ -1281,8 +1285,10 @@ static void http_step ( struct http_request *http ) {
  err_auth:
 	free ( range );
  err_range:
-	free ( uri );
+	free ( path_uri_string );
- err_uri:
+ err_path_uri:
+	free ( host_uri_string );
+ err_host_uri:
 	if ( rc != 0 )
 		http_close ( http, rc );
 }

src/net/udp/tftp.c

@@ -323,24 +323,12 @@ void tftp_set_mtftp_port ( unsigned int port ) {
  * @ret rc		Return status code
  */
 static int tftp_send_rrq ( struct tftp_request *tftp ) {
+	const char *path = tftp->uri->path;
 	struct tftp_rrq *rrq;
-	const char *path;
 	size_t len;
 	struct io_buffer *iobuf;
 	size_t blksize;
 
-	/* Strip initial '/' if present.  If we were opened via the
-	 * URI interface, then there will be an initial '/', since a
-	 * full tftp:// URI provides no way to specify a non-absolute
-	 * path.  However, many TFTP servers (particularly Windows
-	 * TFTP servers) complain about having an initial '/', and it
-	 * violates user expectations to have a '/' silently added to
-	 * the DHCP-specified filename.
-	 */
-	path = tftp->uri->path;
-	if ( *path == '/' )
-		path++;
-
 	DBGC ( tftp, "TFTP %p requesting \"%s\"\n", tftp, path );
 
 	/* Allocate buffer */

src/tests/tests.c

@@ -53,3 +53,4 @@ REQUIRE_OBJECT ( pnm_test );
 REQUIRE_OBJECT ( deflate_test );
 REQUIRE_OBJECT ( png_test );
 REQUIRE_OBJECT ( dns_test );
+REQUIRE_OBJECT ( uri_test );

src/tests/uri_test.c

@@ -1,146 +1,858 @@
-#include <stdint.h>
+/*
-#include <stddef.h>
+ * Copyright (C) 2014 Michael Brown <mbrown@fensystems.co.uk>.
-#include <stdio.h>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+
+FILE_LICENCE ( GPL2_OR_LATER );
+
+/** @file
+ *
+ * URI self-tests
+ *
+ */
+
+/* Forcibly enable assertions */
+#undef NDEBUG
+
 #include <string.h>
-#include <errno.h>
+#include <byteswap.h>
 #include <ipxe/uri.h>
+#include <ipxe/params.h>
+#include <ipxe/test.h>
 
-#define URI_MAX_LEN 1024
+/** A URI parsing/formatting test */
-
 struct uri_test {
-	const char *base_uri_string;
+	/** URI string */
-	const char *relative_uri_string;
+	const char *string;
-	const char *resolved_uri_string;
+	/** URI */
+	struct uri uri;
 };
 
-static struct uri_test uri_tests[] = {
+/** A URI port number test */
-	{ "http://www.fensystems.co.uk", "",
+struct uri_port_test {
-	  "http://www.fensystems.co.uk/" },
+	/** URI string */
-	{ "http://ipxe.org/wiki/page1", "page2",
+	const char *string;
-	  "http://ipxe.org/wiki/page2" },
+	/** Default port number */
-	{ "http://ipxe.org/wiki/page1", "../page3",
+	unsigned int default_port;
-	  "http://ipxe.org/page3" },
+	/** Expected port number */
-	{ "tftp://192.168.0.1/", "/tftpboot/vmlinuz",
+	unsigned int port;
-	  "tftp://192.168.0.1/tftpboot/vmlinuz" },
-	{ "ftp://the%41nswer%3d:%34ty%32wo@ether%62oot.org:8080/p%41th/foo",
-	  "to?%41=b#%43d",
-	  "ftp://theAnswer%3d:4ty2wo@ipxe.org:8080/path/to?a=b#cd" },
-#if 0
-	"http://www.ipxe.org/wiki",
-	"mailto:bob@nowhere.com",
-	"ftp://joe:secret@insecure.org:8081/hidden/path/to?what=is#this",
-#endif
 };
 
-static int test_parse_unparse ( const char *uri_string ) {
+/** A URI or path resolution test */
-	char buf[URI_MAX_LEN];
+struct uri_resolve_test {
-	struct uri *uri = NULL;
+	/** Base path or URI */
-	int rc;
+	const char *base;
+	/** Relative path or URI */
+	const char *relative;
+	/** Expected resolved path or URI */
+	const char *resolved;
+};
 
-	/* Parse and unparse URI */
+/** A TFTP URI test */
-	uri = parse_uri ( uri_string );
+struct uri_tftp_test {
-	if ( ! uri ) {
+	/** Next-server address */
-		rc = -ENOMEM;
+	struct in_addr next_server;
-		goto done;
+	/** Filename */
+	const char *filename;
+	/** URI */
+	struct uri uri;
+	/** URI string (for display only; cannot be reparsed) */
+	const char *string;
+};
+
+/** A current working URI test */
+struct uri_churi_test {
+	/** Relative URI */
+	const char *relative;
+	/** Expected new working URI */
+	const char *expected;
+};
+
+/** A form parameter URI test list */
+struct uri_params_test_list {
+	/** Key */
+	const char *key;
+	/** Value */
+	const char *value;
+};
+
+/** A form parameter URI test */
+struct uri_params_test {
+	/** URI string */
+	const char *string;
+	/** URI */
+	struct uri uri;
+	/** Parameter list name */
+	const char *name;
+	/** Parameter list */
+	struct uri_params_test_list *list;
+};
+
+/**
+ * Compare two URI component strings
+ *
+ * @v first		First string, or NULL
+ * @v second		Second string, or NULL
+ * @v difference	Difference
+ */
+static int uristrcmp ( const char *first, const char *second ) {
+
+	/* Compare strings, allowing for either to be NULL */
+	if ( first == second ) {
+		return 0;
+	} else if ( ( first == NULL ) || ( second == NULL ) ) {
+		return -1;
+	} else {
+		return strcmp ( first, second );
 	}
-	unparse_uri ( buf, sizeof ( buf ), uri, URI_ALL );
-
-	/* Compare result */
-	if ( strcmp ( buf, uri_string ) != 0 ) {
-		printf ( "Unparse of \"%s\" produced \"%s\"\n",
-			 uri_string, buf );
-		rc = -EINVAL;
-		goto done;
 }
 
-	rc = 0;
+/**
+ * Report URI equality test result
+ *
+ * @v uri		URI
+ * @v expected		Expected URI
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_okx ( struct uri *uri, struct uri *expected, const char *file,
+		      unsigned int line ) {
 
- done:
+	okx ( uristrcmp ( uri->scheme, expected->scheme ) == 0, file, line );
+	okx ( uristrcmp ( uri->opaque, expected->opaque ) == 0, file, line );
+	okx ( uristrcmp ( uri->user, expected->user ) == 0, file, line );
+	okx ( uristrcmp ( uri->password, expected->password ) == 0, file, line);
+	okx ( uristrcmp ( uri->host, expected->host ) == 0, file, line );
+	okx ( uristrcmp ( uri->port, expected->port ) == 0, file, line );
+	okx ( uristrcmp ( uri->path, expected->path ) == 0, file, line );
+	okx ( uristrcmp ( uri->query, expected->query ) == 0, file, line );
+	okx ( uristrcmp ( uri->fragment, expected->fragment ) == 0, file, line);
+	okx ( uri->params == expected->params, file, line );
+}
+#define uri_ok( uri, expected ) uri_okx ( uri, expected, __FILE__, __LINE__ )
+
+/**
+ * Report URI parsing test result
+ *
+ * @v test		URI test
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_parse_okx ( struct uri_test *test, const char *file,
+			    unsigned int line ) {
+	struct uri *uri;
+
+	/* Parse URI */
+	uri = parse_uri ( test->string );
+	okx ( uri != NULL, file, line );
+	if ( uri )
+		uri_okx ( uri, &test->uri, file, line );
 	uri_put ( uri );
-	if ( rc ) {
-		printf ( "URI parse-unparse of \"%s\" failed: %s\n",
-			 uri_string, strerror ( rc ) );
-	}
-	return rc;
 }
+#define uri_parse_ok( test ) uri_parse_okx ( test, __FILE__, __LINE__ )
 
-static int test_resolve ( const char *base_uri_string,
+/**
-			  const char *relative_uri_string,
+ * Report URI formatting test result
-			  const char *resolved_uri_string ) {
+ *
-	struct uri *base_uri = NULL;
+ * @v test		URI test
-	struct uri *relative_uri = NULL;
+ * @v file		Test code file
-	struct uri *resolved_uri = NULL;
+ * @v line		Test code line
-	char buf[URI_MAX_LEN];
+ */
-	int rc;
+static void uri_format_okx ( struct uri_test *test, const char *file,
+			     unsigned int line ) {
+	char buf[ strlen ( test->string ) + 1 /* NUL */ ];
+	char *tmp;
+	size_t len;
+
+	/* Format into fixed-size buffer */
+	len = format_uri ( &test->uri, buf, sizeof ( buf ) );
+	okx ( len == ( sizeof ( buf ) - 1 /* NUL */ ), file, line );
+	okx ( strcmp ( buf, test->string ) == 0, file, line );
+
+	/* Format into temporarily allocated buffer */
+	tmp = format_uri_alloc ( &test->uri );
+	okx ( tmp != NULL, file, line );
+	if ( tmp )
+		okx ( strcmp ( tmp, test->string ) == 0, file, line );
+	free ( tmp );
+}
+#define uri_format_ok( test ) uri_format_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report URI duplication test result
+ *
+ * @v test		URI
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_dup_okx ( struct uri *uri, const char *file,
+			  unsigned int line ) {
+	struct uri *dup;
+
+	dup = uri_dup ( uri );
+	okx ( dup != NULL, file, line );
+	if ( dup )
+		uri_okx ( dup, uri, file, line );
+	uri_put ( dup );
+}
+#define uri_dup_ok( test ) uri_dup_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report URI combined parsing and formatting test result
+ *
+ * @v test		URI test
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_parse_format_dup_okx ( struct uri_test *test, const char *file,
+				       unsigned int line ) {
+
+	uri_parse_okx ( test, file, line );
+	uri_format_okx ( test, file, line );
+	uri_dup_okx ( &test->uri, file, line );
+}
+#define uri_parse_format_dup_ok( test ) \
+	uri_parse_format_dup_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report URI port number test result
+ *
+ * @v test		URI port number test
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_port_okx ( struct uri_port_test *test, const char *file,
+			   unsigned int line ) {
+	struct uri *uri;
+	unsigned int port;
+
+	/* Parse URI */
+	uri = parse_uri ( test->string );
+	okx ( uri != NULL, file, line );
+	if ( uri ) {
+		port = uri_port ( uri, test->default_port );
+		okx ( port == test->port, file, line );
+	}
+	uri_put ( uri );
+}
+#define uri_port_ok( test ) uri_port_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report URI resolution test result
+ *
+ * @v test		Path resolution test
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_resolve_okx ( struct uri_resolve_test *test,
+			      const char *file, unsigned int line ) {
+	struct uri *base;
+	struct uri *relative;
+	struct uri *resolved = NULL;
+	char *formatted;
 
 	/* Parse URIs */
-	base_uri = parse_uri ( base_uri_string );
+	base = parse_uri ( test->base );
-	if ( ! base_uri ) {
+	okx ( base != NULL, file, line );
-		rc = -ENOMEM;
+	relative = parse_uri ( test->relative );
-		goto done;
+	okx ( relative != NULL, file, line );
-	}
-	relative_uri = parse_uri ( relative_uri_string );
-	if ( ! relative_uri ) {
-		rc = -ENOMEM;
-		goto done;
-	}
 
 	/* Resolve URI  */
-	resolved_uri = resolve_uri ( base_uri, relative_uri );
+	if ( base && relative ) {
-	if ( ! resolved_uri ) {
+		resolved = resolve_uri ( base, relative );
-		rc = -ENOMEM;
+		okx ( resolved != NULL, file, line );
-		goto done;
 	}
 
-	/* Compare result */
+	/* Format resolved URI */
-	unparse_uri ( buf, sizeof ( buf ), resolved_uri, URI_ALL );
+	formatted = format_uri_alloc ( resolved );
-	if ( strcmp ( buf, resolved_uri_string ) != 0 ) {
+	okx ( formatted != NULL, file, line );
-		printf ( "Resolution of \"%s\"+\"%s\" produced \"%s\"\n",
+
-			 base_uri_string, relative_uri_string, buf );
+	/* Check resolved URI */
-		rc = -EINVAL;
+	if ( formatted )
-		goto done;
+		okx ( strcmp ( formatted, test->resolved ) == 0, file, line );
+
+	free ( formatted );
+	uri_put ( resolved );
+	uri_put ( relative );
+	uri_put ( base );
+}
+#define uri_resolve_ok( test ) uri_resolve_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report path resolution test result
+ *
+ * @v test		Path resolution test
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_resolve_path_okx ( struct uri_resolve_test *test,
+				   const char *file, unsigned int line ) {
+	char *resolved;
+
+	/* Resolve paths using resolve_path() directly */
+	resolved = resolve_path ( test->base, test->relative );
+	okx ( resolved != NULL, file, line );
+	if ( resolved )
+		okx ( strcmp ( resolved, test->resolved ) == 0, file, line );
+	free ( resolved );
+
+	/* Resolve paths as URIs (since all paths are valid URIs) */
+	uri_resolve_okx ( test, file, line );
+}
+#define uri_resolve_path_ok( test ) \
+	uri_resolve_path_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report URI TFTP test result
+ *
+ * @v test		URI TFTP test
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_tftp_okx ( struct uri_tftp_test *test, const char *file,
+			   unsigned int line ) {
+	char buf[ strlen ( test->string ) + 1 /* NUL */ ];
+	struct uri *uri;
+	size_t len;
+
+	/* Construct URI */
+	uri = tftp_uri ( test->next_server, test->filename );
+	okx ( uri != NULL, file, line );
+	if ( uri ) {
+		uri_okx ( uri, &test->uri, file, line );
+		len = format_uri ( uri, buf, sizeof ( buf ) );
+		okx ( len == ( sizeof ( buf ) - 1 /* NUL */ ), file, line );
+		okx ( strcmp ( buf, test->string ) == 0, file, line );
+	}
+	uri_put ( uri );
+}
+#define uri_tftp_ok( test ) uri_tftp_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report current working URI test result
+ *
+ * @v tests		List of current working URI tests
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_churi_okx ( struct uri_churi_test *test, const char *file,
+			    unsigned int line ) {
+	struct uri *old_cwuri;
+	struct uri *uri;
+	char *formatted;
+
+	/* Preserve original current working URI */
+	old_cwuri = uri_get ( cwuri );
+
+	/* Perform sequence of current working URI changes */
+	do {
+		/* Parse relative URI */
+		uri = parse_uri ( test->relative );
+		okx ( uri != NULL, file, line );
+
+		/* Move to this URI */
+		churi ( uri );
+
+		/* Format new current working URI */
+		formatted = format_uri_alloc ( cwuri );
+		okx ( formatted != NULL, file, line );
+		if ( formatted ) {
+			okx ( strcmp ( formatted, test->expected ) == 0,
+			      file, line );
 		}
 
-	rc = 0;
+		/* Free temporary storage */
+		free ( formatted );
+		uri_put ( uri );
 
- done:
+		/* Move to next current working URI test */
-	uri_put ( base_uri );
+		test++;
-	uri_put ( relative_uri );
+
-	uri_put ( resolved_uri );
+	} while ( test->relative != NULL );
-	if ( rc ) {
+
-		printf ( "URI resolution of \"%s\"+\"%s\" failed: %s\n",
+	/* Restore original current working URI */
-			 base_uri_string, relative_uri_string,
+	churi ( old_cwuri );
-			 strerror ( rc ) );
+	uri_put ( old_cwuri );
+}
+#define uri_churi_ok( test ) uri_churi_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report form parameter URI test list result
+ *
+ * @v test		Form parameter URI test
+ * @v uri		URI
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_params_list_okx ( struct uri_params_test *test,
+				  struct uri *uri, const char *file,
+				  unsigned int line ) {
+	struct uri_params_test_list *list;
+	struct parameter *param;
+
+	/* Check URI */
+	uri_okx ( uri, &test->uri, file, line );
+
+	/* Check URI parameters */
+	okx ( uri->params != NULL, file, line );
+	if ( uri->params ) {
+		list = test->list;
+		for_each_param ( param, uri->params ) {
+			okx ( strcmp ( param->key, list->key ) == 0,
+			      file, line );
+			okx ( strcmp ( param->value, list->value ) == 0,
+			      file, line );
+			list++;
+		}
+		okx ( list->key == NULL, file, line );
+	}
+}
+#define uri_params_list_ok( test ) \
+	uri_params_list_okx ( test, __FILE__, __LINE__ )
+
+/**
+ * Report form parameter URI test result
+ *
+ * @v test		Form parameter URI test
+ * @v file		Test code file
+ * @v line		Test code line
+ */
+static void uri_params_okx ( struct uri_params_test *test, const char *file,
+			     unsigned int line ) {
+	struct uri_params_test_list *list;
+	struct parameters *params;
+	struct parameter *param;
+	struct uri *uri;
+	struct uri *dup;
+
+	/* Create parameter list */
+	params = create_parameters ( test->name );
+	okx ( params != NULL, file, line );
+	if ( params ) {
+		for ( list = test->list ; list->key ; list++ ) {
+			param = add_parameter ( params, list->key, list->value);
+			okx ( param != NULL, file, line );
 		}
-	return rc;
 	}
 
-int uri_test ( void ) {
+	/* Record parameter list as part of expected URI */
-	unsigned int i;
+	test->uri.params = params;
-	struct uri_test *uri_test;
-	int rc;
-	int overall_rc = 0;
 
-	for ( i = 0 ; i < ( sizeof ( uri_tests ) /
+	/* Parse URI */
-			    sizeof ( uri_tests[0] ) ) ; i++ ) {
+	uri = parse_uri ( test->string );
-		uri_test = &uri_tests[i];
+	okx ( uri != NULL, file, line );
-		rc = test_parse_unparse ( uri_test->base_uri_string );
+	if ( uri )
-		if ( rc != 0 )
+		uri_params_list_okx ( test, uri, file, line );
-			overall_rc = rc;
+
-		rc = test_parse_unparse ( uri_test->relative_uri_string );
+	/* Duplicate URI */
-		if ( rc != 0 )
+	dup = uri_dup ( uri );
-			overall_rc = rc;
+	okx ( dup != NULL, file, line );
-		rc = test_parse_unparse ( uri_test->resolved_uri_string );
+	if ( dup )
-		if ( rc != 0 )
+		uri_params_list_okx ( test, dup, file, line );
-			overall_rc = rc;
+
-		rc = test_resolve ( uri_test->base_uri_string,
+	/* Clear parameter list in expected URI */
-				    uri_test->relative_uri_string,
+	test->uri.params = NULL;
-				    uri_test->resolved_uri_string );
+
-		if ( rc != 0 )
+	uri_put ( uri );
-			overall_rc = rc;
+	uri_put ( dup );
+}
+#define uri_params_ok( test ) uri_params_okx ( test, __FILE__, __LINE__ )
+
+/** Empty URI */
+static struct uri_test uri_empty = {
+	.string = "",
+};
+
+/** Basic HTTP URI */
+static struct uri_test uri_boot_ipxe_org = {
+	"http://boot.ipxe.org/demo/boot.php",
+	{ .scheme = "http", .host = "boot.ipxe.org", .path = "/demo/boot.php" }
+};
+
+/** Basic opaque URI */
+static struct uri_test uri_mailto = {
+	"mailto:ipxe-devel@lists.ipxe.org",
+	{ .scheme = "mailto", .opaque = "ipxe-devel@lists.ipxe.org" }
+};
+
+/** HTTP URI with all the trimmings */
+static struct uri_test uri_http_all = {
+	"http://anon:password@example.com:3001/~foo/cgi-bin/foo.pl?a=b&c=d#bit",
+	{
+		.scheme = "http",
+		.user = "anon",
+		.password = "password",
+		.host = "example.com",
+		.port = "3001",
+		.path = "/~foo/cgi-bin/foo.pl",
+		.query = "a=b&c=d",
+		.fragment = "bit",
+	},
+};
+
+/** HTTP URI with escaped characters */
+static struct uri_test uri_http_escaped = {
+	"https://test.ipxe.org/wtf%3F%0A?kind%23of/uri%20is#this%3F",
+	{
+		.scheme = "https",
+		.host = "test.ipxe.org",
+		.path = "/wtf?\n",
+		.query = "kind#of/uri is",
+		.fragment = "this?",
+	},
+};
+
+/** HTTP URI with improperly escaped characters */
+static struct uri_test uri_http_escaped_improper = {
+	/* We accept for parsing improperly escaped characters.
+	 * (Formatting the parsed URI would produce the properly
+	 * encoded form, and so would not exactly match the original
+	 * URI string.)
+	 */
+	"https://test%2eipxe.org/wt%66%3f\n?kind%23of/uri is#this?",
+	{
+		.scheme = "https",
+		.host = "test.ipxe.org",
+		.path = "/wtf?\n",
+		.query = "kind#of/uri is",
+		.fragment = "this?",
+	},
+};
+
+/** IPv6 URI */
+static struct uri_test uri_ipv6 = {
+	"http://[2001:ba8:0:1d4::6950:5845]/",
+	{
+		.scheme = "http",
+		.host = "[2001:ba8:0:1d4::6950:5845]",
+		.path = "/",
+	},
+};
+
+/** IPv6 URI with port */
+static struct uri_test uri_ipv6_port = {
+	"http://[2001:ba8:0:1d4::6950:5845]:8001/boot",
+	{
+		.scheme = "http",
+		.host = "[2001:ba8:0:1d4::6950:5845]",
+		.port = "8001",
+		.path = "/boot",
+	},
+};
+
+/** IPv6 URI with link-local address */
+static struct uri_test uri_ipv6_local = {
+	"http://[fe80::69ff:fe50:5845%25net0]/ipxe",
+	{
+		.scheme = "http",
+		.host = "[fe80::69ff:fe50:5845%net0]",
+		.path = "/ipxe",
+	},
+};
+
+/** IPv6 URI with link-local address not conforming to RFC 6874 */
+static struct uri_test uri_ipv6_local_non_conforming = {
+	/* We accept for parsing a single "%" in "%net0" (rather than
+	 * the properly encoded form "%25net0").  (Formatting the
+	 * parsed URI would produce the properly encoded form, and so
+	 * would not exactly match the original URI string.)
+	 */
+	"http://[fe80::69ff:fe50:5845%net0]/ipxe",
+	{
+		.scheme = "http",
+		.host = "[fe80::69ff:fe50:5845%net0]",
+		.path = "/ipxe",
+	},
+};
+
+/** iSCSI URI */
+static struct uri_test uri_iscsi = {
+	"iscsi:10.253.253.1::::iqn.2010-04.org.ipxe:rabbit",
+	{
+		.scheme = "iscsi",
+		.opaque = "10.253.253.1::::iqn.2010-04.org.ipxe:rabbit",
+	},
+};
+
+/** URI with port number */
+static struct uri_port_test uri_explicit_port = {
+	"http://192.168.0.1:8080/boot.php",
+	80,
+	8080,
+};
+
+/** URI without port number */
+static struct uri_port_test uri_default_port = {
+	"http://192.168.0.1/boot.php",
+	80,
+	80,
+};
+
+/** Simple path resolution test */
+static struct uri_resolve_test uri_simple_path = {
+	"/etc/passwd",
+	"group",
+	"/etc/group",
+};
+
+/** Path resolution test with "." and ".." elements */
+static struct uri_resolve_test uri_relative_path = {
+	"/var/lib/tftpboot/pxe/pxelinux.0",
+	"./../ipxe/undionly.kpxe",
+	"/var/lib/tftpboot/ipxe/undionly.kpxe",
+};
+
+/** Path resolution test terminating with directory */
+static struct uri_resolve_test uri_directory_path = {
+	"/test/cgi-bin.pl/boot.ipxe",
+	"..",
+	"/test/",
+};
+
+/** Path resolution test with excessive ".." elements */
+static struct uri_resolve_test uri_excessive_path = {
+	"/var/lib/tftpboot/ipxe.pxe",
+	"../../../../../../../foo",
+	"/foo",
+};
+
+/** Path resolution test with absolute path */
+static struct uri_resolve_test uri_absolute_path = {
+	"/var/lib/tftpboot",
+	"/etc/hostname",
+	"/etc/hostname",
+};
+
+/** Relative URI resolution test */
+static struct uri_resolve_test uri_relative = {
+	"http://boot.ipxe.org/demo/boot.php?vendor=10ec&device=8139",
+	"initrd.img",
+	"http://boot.ipxe.org/demo/initrd.img",
+};
+
+/** Absolute URI resolution test */
+static struct uri_resolve_test uri_absolute = {
+	"http://boot.ipxe.org/demo/boot.php",
+	"ftp://192.168.0.1/boot.ipxe",
+	"ftp://192.168.0.1/boot.ipxe",
+};
+
+/** Absolute path URI resolution test */
+static struct uri_resolve_test uri_absolute_uri_path = {
+	"http://boot.ipxe.org/demo/boot.php#test",
+	"/demo/vmlinuz",
+	"http://boot.ipxe.org/demo/vmlinuz",
+};
+
+/** Query URI resolution test */
+static struct uri_resolve_test uri_query = {
+	"http://10.253.253.1/test.pl?mac=02-00-69-50-58-45",
+	"?mac=00-1f-16-bc-fe-2f",
+	"http://10.253.253.1/test.pl?mac=00-1f-16-bc-fe-2f",
+};
+
+/** Fragment URI resolution test */
+static struct uri_resolve_test uri_fragment = {
+	"http://192.168.0.254/test#foo",
+	"#bar",
+	"http://192.168.0.254/test#bar",
+};
+
+/** TFTP URI with absolute path */
+static struct uri_tftp_test uri_tftp_absolute = {
+	{ .s_addr = htonl ( 0xc0a80002 ) /* 192.168.0.2 */ },
+	"/absolute/path",
+	{
+		.scheme = "tftp",
+		.host = "192.168.0.2",
+		.path = "/absolute/path",
+	},
+	"tftp://192.168.0.2/absolute/path",
+};
+
+/** TFTP URI with relative path */
+static struct uri_tftp_test uri_tftp_relative = {
+	{ .s_addr = htonl ( 0xc0a80003 ) /* 192.168.0.3 */ },
+	"relative/path",
+	{
+		.scheme = "tftp",
+		.host = "192.168.0.3",
+		.path = "relative/path",
+	},
+	"tftp://192.168.0.3/relative/path",
+};
+
+/** TFTP URI with path containing special characters */
+static struct uri_tftp_test uri_tftp_icky = {
+	{ .s_addr = htonl ( 0x0a000006 ) /* 10.0.0.6 */ },
+	"C:\\tftpboot\\icky#path",
+	{
+		.scheme = "tftp",
+		.host = "10.0.0.6",
+		.path = "C:\\tftpboot\\icky#path",
+	},
+	"tftp://10.0.0.6/C%3A\\tftpboot\\icky%23path",
+};
+
+/** Current working URI test */
+static struct uri_churi_test uri_churi[] = {
+	{
+		"http://boot.ipxe.org/demo/boot.php",
+		"http://boot.ipxe.org/demo/boot.php",
+	},
+	{
+		"?vendor=10ec&device=8139",
+		"http://boot.ipxe.org/demo/boot.php?vendor=10ec&device=8139",
+	},
+	{
+		"fedora/fedora.ipxe",
+		"http://boot.ipxe.org/demo/fedora/fedora.ipxe",
+	},
+	{
+		"vmlinuz",
+		"http://boot.ipxe.org/demo/fedora/vmlinuz",
+	},
+	{
+		"http://local/boot/initrd.img",
+		"http://local/boot/initrd.img",
+	},
+	{
+		"modules/8139too.ko",
+		"http://local/boot/modules/8139too.ko",
+	},
+	{
+		NULL,
+		NULL,
+	}
+};
+
+/** Form parameter URI test list */
+static struct uri_params_test_list uri_params_list[] = {
+	{
+		"vendor",
+		"10ec",
+	},
+	{
+		"device",
+		"8139",
+	},
+	{
+		"uuid",
+		"f59fac00-758f-498f-9fe5-87d790045d94",
+	},
+	{
+		NULL,
+		NULL,
+	}
+};
+
+/** Form parameter URI test */
+static struct uri_params_test uri_params = {
+	"http://boot.ipxe.org/demo/boot.php##params",
+	{
+		.scheme = "http",
+		.host = "boot.ipxe.org",
+		.path = "/demo/boot.php",
+	},
+	NULL,
+	uri_params_list,
+};
+
+/** Named form parameter URI test list */
+static struct uri_params_test_list uri_named_params_list[] = {
+	{
+		"mac",
+		"00:1e:65:80:d3:b6",
+	},
+	{
+		"serial",
+		"LXTQ20Z1139322762F2000",
+	},
+	{
+		NULL,
+		NULL,
+	}
+};
+
+/** Named form parameter URI test */
+static struct uri_params_test uri_named_params = {
+	"http://192.168.100.4:3001/register##params=foo",
+	{
+		.scheme = "http",
+		.host = "192.168.100.4",
+		.port = "3001",
+		.path = "/register",
+	},
+	"foo",
+	uri_named_params_list,
+};
+
+/**
+ * Perform URI self-test
+ *
+ */
+static void uri_test_exec ( void ) {
+
+	/* URI parsing, formatting, and duplication tests */
+	uri_parse_format_dup_ok ( &uri_empty );
+	uri_parse_format_dup_ok ( &uri_boot_ipxe_org );
+	uri_parse_format_dup_ok ( &uri_mailto );
+	uri_parse_format_dup_ok ( &uri_http_all );
+	uri_parse_format_dup_ok ( &uri_http_escaped );
+	uri_parse_ok ( &uri_http_escaped_improper ); /* Parse only */
+	uri_parse_format_dup_ok ( &uri_ipv6 );
+	uri_parse_format_dup_ok ( &uri_ipv6_port );
+	uri_parse_format_dup_ok ( &uri_ipv6_local );
+	uri_parse_ok ( &uri_ipv6_local_non_conforming ); /* Parse only */
+	uri_parse_format_dup_ok ( &uri_iscsi );
+
+	/** URI port number tests */
+	uri_port_ok ( &uri_explicit_port );
+	uri_port_ok ( &uri_default_port );
+
+	/** Path resolution tests */
+	uri_resolve_path_ok ( &uri_simple_path );
+	uri_resolve_path_ok ( &uri_relative_path );
+	uri_resolve_path_ok ( &uri_directory_path );
+	uri_resolve_path_ok ( &uri_excessive_path );
+	uri_resolve_path_ok ( &uri_absolute_path );
+
+	/** URI resolution tests */
+	uri_resolve_ok ( &uri_relative );
+	uri_resolve_ok ( &uri_absolute );
+	uri_resolve_ok ( &uri_absolute_uri_path );
+	uri_resolve_ok ( &uri_query );
+	uri_resolve_ok ( &uri_fragment );
+
+	/* TFTP URI construction tests */
+	uri_tftp_ok ( &uri_tftp_absolute );
+	uri_tftp_ok ( &uri_tftp_relative );
+	uri_tftp_ok ( &uri_tftp_icky );
+
+	/* Current working URI tests */
+	uri_churi_ok ( uri_churi );
+
+	/* Form parameter URI tests */
+	uri_params_ok ( &uri_params );
+	uri_params_ok ( &uri_named_params );
 }
 
-	if ( overall_rc )
+/** URI self-test */
-		printf ( "URI tests failed: %s\n", strerror ( overall_rc ) );
+struct self_test uri_test __self_test = {
-	return overall_rc;
+	.name = "uri",
-}
+	.exec = uri_test_exec,
+};

src/usr/autoboot.c

@@ -91,8 +91,6 @@ static struct net_device * find_boot_netdev ( void ) {
  */
 static struct uri * parse_next_server_and_filename ( struct in_addr next_server,
 						     const char *filename ) {
-	char buf[ 23 /* "tftp://xxx.xxx.xxx.xxx/" */ + strlen ( filename )
-		  + 1 /* NUL */ ];
 	struct uri *uri;
 
 	/* Parse filename */
@@ -100,17 +98,10 @@ static struct uri * parse_next_server_and_filename ( struct in_addr next_server,
 	if ( ! uri )
 		return NULL;
 
-	/* Construct a tftp:// URI for the filename, if applicable.
+	/* Construct a TFTP URI for the filename, if applicable */
-	 * We can't just rely on the current working URI, because the
-	 * relative URI resolution will remove the distinction between
-	 * filenames with and without initial slashes, which is
-	 * significant for TFTP.
-	 */
 	if ( next_server.s_addr && filename[0] && ! uri_is_absolute ( uri ) ) {
 		uri_put ( uri );
-		snprintf ( buf, sizeof ( buf ), "tftp://%s/%s",
+		uri = tftp_uri ( next_server, filename );
-			   inet_ntoa ( next_server ), filename );
-		uri = parse_uri ( buf );
 		if ( ! uri )
 			return NULL;
 	}

src/usr/imgmgmt.c

@@ -44,9 +44,8 @@ FILE_LICENCE ( GPL2_OR_LATER );
  * @ret rc		Return status code
  */
 int imgdownload ( struct uri *uri, struct image **image ) {
-	size_t len = ( unparse_uri ( NULL, 0, uri, URI_ALL ) + 1 );
-	char uri_string_redacted[len];
 	const char *password;
+	char *uri_string_redacted;
 	int rc;
 
 	/* Allocate image */
@@ -56,13 +55,16 @@ int imgdownload ( struct uri *uri, struct image **image ) {
 		goto err_alloc_image;
 	}
 
-	/* Redact password portion of URI, if necessary */
+	/* Construct redacted URI */
 	password = uri->password;
 	if ( password )
 		uri->password = "***";
-	unparse_uri ( uri_string_redacted, sizeof ( uri_string_redacted ),
+	uri_string_redacted = format_uri_alloc ( uri );
-		      uri, URI_ALL );
 	uri->password = password;
+	if ( ! uri_string_redacted ) {
+		rc = -ENOMEM;
+		goto err_uri;
+	}
 
 	/* Create downloader */
 	if ( ( rc = create_downloader ( &monojob, *image, LOCATION_URI,
@@ -81,16 +83,11 @@ int imgdownload ( struct uri *uri, struct image **image ) {
 		goto err_register_image;
 	}
 
-	/* Drop local reference to image.  Image is guaranteed to
-	 * remain in scope since it is registered.
-	 */
-	image_put ( *image );
-
-	return 0;
-
  err_register_image:
  err_monojob_wait:
  err_create_downloader:
+	free ( uri_string_redacted );
+ err_uri:
 	image_put ( *image );
  err_alloc_image:
 	return rc;