Syslinux - Jun 2013 - [5.10] PXE + dhcp opts 209, 210 and path issues in tftp/http

On Mon, 10 Jun, at 07:57:50AM, H. Peter Anvin wrote:
> Either that or make the path a list rather than a string, using the
> normal word separators when entered on the command line, a bit like the
> (t)csh does.  That is a bigger change but is probably a better solution.
 
How would this solution handle filenames containing spaces? Would we
need to escape (presumably with a backslash) such filenames?

-- 
Matt Fleming, Intel Open Source Technology Center

On 06/11/2013 01:03 AM, Matt Fleming wrote:
> On Mon, 10 Jun, at 07:57:50AM, H. Peter Anvin wrote:
>> Either that or make the path a list rather than a string, using the
>> normal word separators when entered on the command line, a bit like the
>> (t)csh does.  That is a bigger change but is probably a better
solution.
>  
> How would this solution handle filenames containing spaces? Would we
> need to escape (presumably with a backslash) such filenames?
> 
Well, we currently don't support filenames with spaces at all.  We would
have to come up with new syntax for that (quoted strings, what have you)
but then perhaps we can use it elsewhere, too.

	-hpa

On Tue, 11 Jun, at 03:54:21AM, H. Peter Anvin wrote:
> On 06/11/2013 01:03 AM, Matt Fleming wrote:
> > On Mon, 10 Jun, at 07:57:50AM, H. Peter Anvin wrote:
> >> Either that or make the path a list rather than a string, using
the
> >> normal word separators when entered on the command line, a bit
like the
> >> (t)csh does.  That is a bigger change but is probably a better
solution.
> >  
> > How would this solution handle filenames containing spaces? Would we
> > need to escape (presumably with a backslash) such filenames?
> > 
> 
> Well, we currently don't support filenames with spaces at all.  We
would
> have to come up with new syntax for that (quoted strings, what have you)
> but then perhaps we can use it elsewhere, too.
OK, how about something like this? It also change the internal PATH into
a linked list instead of requiring us to parse a string constantly;
which was complete madness.

---
>From a78a64f06c1092c7e0e060be69f8aaf9679a6400 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming at intel.com>
Date: Wed, 12 Jun 2013 13:04:44 +0100
Subject: [PATCH] PATH: Change the PATH directive syntax

In retrospect, choosing the colon character as the entry separator for
the PATH directive was not a smart move, as that character is also used
in TFTP-style paths. This conflict manifests as PXELINUX being unable to
find and load files.

An example dnsmasq log looks like,

dnsmasq-tftp: sent /arch/boot/syslinux/lpxelinux.0 to 192.168.0.90
dnsmasq-tftp: file /arch/ldlinux.c32 not found
dnsmasq-tftp: file /arch//ldlinux.c32 not found
dnsmasq-tftp: file /arch//boot/isolinux/ldlinux.c32 not found
dnsmasq-tftp: file /arch//isolinux/ldlinux.c32 not found
dnsmasq-tftp: file /arch//boot/syslinuxldlinux.c32 not found
dnsmasq-tftp: sent /arch//boot/syslinux/ldlinux.c32 to 192.168.0.90
dnsmasq-tftp: error 0 No error, file close received from 192.168.0.90
dnsmasq-tftp: failed sending /arch//boot/syslinux/ldlinux.c32 to 192.168.0.90
dnsmasq-tftp: sent /arch/boot/syslinux/archiso.cfg to 192.168.0.90
dnsmasq-tftp: sent /arch/boot/syslinux/whichsys.c32 to 192.168.0.90
dnsmasq-tftp: file /arch/libcom32.c32 not found
dnsmasq-tftp: file /arch//libcom32.c32 not found
dnsmasq-tftp: file /arch/libcom32.c32 not found
dnsmasq-tftp: file /arch//arch//boot/syslinux/libcom32.c32 not found

The last line of the log is the indication that there's a problem.
Internally, Syslinux adds the location of ldlinux.c32 to PATH by
querying the current working directory once ldlinux.c32 is successfully
loaded. Under PXELINUX that means the initial PATH string will be,

   "::/arch/boot/syslinux/"

The PATH parsing code doesn't know how to correctly parse the "::"
string and hence, the file is searched for relative to the 210 dhcp
option directory - /arch/.

Switch to using the space character to separate PATH entries instead,
which not only allows TFTP-style paths but also http and ftp urls. If we
ever support filenames containing spaces in config files, then we'll
have to introduce some means of quotation, to distinguish filenames
containing spaces from separate PATH entries.

Internally, PATH is now a linked list which *greatly* simplifies the
path code, and means we no longer have to parse strings backwards and
forwards.

Note: This change is not backwards-compatible, which means that anyone
using the PATH directive with versions before 5.11 will need to change
their config file when upgrading.

Signed-off-by: Matt Fleming <matt.fleming at intel.com>
---
 com32/elflink/ldlinux/readconfig.c | 53 +++++++++++++++++++++++++-------------
 com32/lib/sys/module/common.c      | 36 +++++++++-----------------
 core/elflink/load_env32.c          | 26 +++----------------
 core/fs/fs.c                       |  2 --
 core/include/fs.h                  | 10 ++++++-
 doc/syslinux.txt                   |  2 +-
 6 files changed, 61 insertions(+), 68 deletions(-)

diff --git a/com32/elflink/ldlinux/readconfig.c
b/com32/elflink/ldlinux/readconfig.c
index 35b137e..12cb7e3 100644
--- a/com32/elflink/ldlinux/readconfig.c
+++ b/com32/elflink/ldlinux/readconfig.c
@@ -770,6 +770,39 @@ extern void loadkeys(char *);
 extern char syslinux_banner[];
 extern char copyright_str[];
 
+/*
+ * PATH-based lookup
+ *
+ * Each entry in the PATH directive is separated by a space, e.g.
+ *
+ *     PATH /bar /bin/foo /baz/bar/bin
+ */
+static int parse_path(char *p)
+{
+    struct path_entry *entry;
+    const char *str;
+
+    while (*p) {
+	/* Find the next directory */
+	str = refdup_word(&p);
+	if (!str)
+	    goto bail;
+
+	entry = path_add(str);
+	refstr_put(str);
+
+	if (!entry)
+	    goto bail;
+
+	p = skipspace(p);
+    }
+
+    return 0;
+
+bail:
+    return -1;
+}
+
 static void parse_config_file(FILE * f)
 {
     char line[MAX_LINE], *p, *ep, ch;
@@ -1337,24 +1370,8 @@ do_include:
 	} else if (looking_at(p, "say")) {
 		printf("%s\n", p+4);
 	} else if (looking_at(p, "path")) {
-		/* PATH-based lookup */
-		const char *new_path;
-		char *_p;
-		size_t len, new_len;
-
-		new_path = refstrdup(skipspace(p + 4));
-		len = strlen(PATH);
-		new_len = strlen(new_path);
-		_p = malloc(len + new_len + 2);
-		if (_p) {
-			strncpy(_p, PATH, len);
-			_p[len++] = ':';
-			strncpy(_p + len, new_path, new_len);
-			_p[len + new_len] = '\0';
-			free(PATH);
-			PATH = _p;
-		} else
-			printf("Failed to realloc PATH\n");
+		if (parse_path(skipspace(p + 4)))
+			printf("Failed to parse PATH\n");
 	} else if (looking_at(p, "sendcookies")) {
 		const union syslinux_derivative_info *sdi;
 
diff --git a/com32/lib/sys/module/common.c b/com32/lib/sys/module/common.c
index 8547036..b763704 100644
--- a/com32/lib/sys/module/common.c
+++ b/com32/lib/sys/module/common.c
@@ -59,40 +59,28 @@ void print_elf_symbols(struct elf_module *module) {
 
 FILE *findpath(char *name)
 {
+	struct path_entry *entry;
 	char path[FILENAME_MAX];
 	FILE *f;
-	char *p, *n;
-	int i;
 
 	f = fopen(name, "rb"); /* for full path */
 	if (f)
 		return f;
 
-	p = PATH;
-again:
-	i = 0;
-	while (*p && *p != ':' && i < FILENAME_MAX - 1) {
-		path[i++] = *p++;
-	}
-
-	if (*p == ':')
-		p++;
+	list_for_each_entry(entry, &PATH, list) {
+		bool slash = false;
 
-	/* Ensure we have a '/' separator */
-	if (path[i] != '/' && i < FILENAME_MAX - 1)
-		path[i++] = '/';
+		/* Ensure we have a '/' separator */
+		if (entry->str[strlen(entry->str) - 1] != '/')
+			slash = true;
 
-	n = name;
-	while (*n && i < FILENAME_MAX - 1)
-		path[i++] = *n++;
-	path[i] = '\0';
+		snprintf(path, sizeof(path), "%s%s%s",
+			 entry->str, slash ? "/" : "", name);
 
-	f = fopen(path, "rb");
-	if (f)
-		return f;
-
-	if (p >= PATH && p < PATH + strlen(PATH))
-		goto again;
+		f = fopen(path, "rb");
+		if (f)
+			return f;
+	}
 
 	return NULL;
 }
diff --git a/core/elflink/load_env32.c b/core/elflink/load_env32.c
index 0483d86..8551831 100644
--- a/core/elflink/load_env32.c
+++ b/core/elflink/load_env32.c
@@ -124,14 +124,11 @@ void load_env32(com32sys_t * regs __unused)
 
 	dprintf("Starting 32 bit elf module subsystem...\n");
 
-	PATH = malloc(strlen(CurrentDirName) + 1);
-	if (!PATH) {
+	if (strlen(CurrentDirName) && !path_add(CurrentDirName)) {
 		printf("Couldn't allocate memory for PATH\n");
 		goto out;
 	}
 
-	strcpy(PATH, CurrentDirName);
-
 	init_module_subsystem(&core_module);
 
 	start_ldlinux(1, argv);
@@ -159,30 +156,15 @@ void load_env32(com32sys_t * regs __unused)
 		if (!core_getcwd(path, sizeof(path)))
 			goto out;
 
-		if (!strlen(PATH)) {
-			PATH = realloc(PATH, strlen(path) + 1);
-			if (!PATH) {
-				printf("Couldn't allocate memory for PATH\n");
-				goto out;
-			}
-
-			strcpy(PATH, path);
-		} else {
-			PATH = realloc(PATH, strlen(path) + strlen(PATH) + 2);
-			if (!PATH) {
-				printf("Couldn't allocate memory for PATH\n");
-				goto out;
-			}
-
-			strcat(PATH, ":");
-			strcat(PATH, path);
+		if (!path_add(path)) {
+			printf("Couldn't allocate memory for PATH\n");
+			goto out;
 		}
 
 		start_ldlinux(1, argv);
 	}
 
 out:
-	free(PATH);
 	writestr("\nFailed to load ldlinux.c32");
 }
 
diff --git a/core/fs/fs.c b/core/fs/fs.c
index 1cb4b00..b6ee19c 100644
--- a/core/fs/fs.c
+++ b/core/fs/fs.c
@@ -10,8 +10,6 @@
 #include "fs.h"
 #include "cache.h"
 
-__export char *PATH;
-
 /* The currently mounted filesystem */
 __export struct fs_info *this_fs = NULL;		/* Root filesystem */
 
diff --git a/core/include/fs.h b/core/include/fs.h
index c7d0fd7..b5c7f0d 100644
--- a/core/include/fs.h
+++ b/core/include/fs.h
@@ -1,6 +1,7 @@
 #ifndef FS_H
 #define FS_H
 
+#include <linux/list.h>
 #include <stddef.h>
 #include <stdbool.h>
 #include <string.h>
@@ -182,7 +183,14 @@ static inline struct file *handle_to_file(uint16_t handle)
     return handle ? &files[handle-1] : NULL;
 }
 
-extern char *PATH;
+struct path_entry {
+    struct list_head list;
+    const char *str;
+};
+
+extern struct list_head PATH;
+
+extern struct path_entry *path_add(const char *str);
 
 /* fs.c */
 void pm_mangle_name(com32sys_t *);
diff --git a/doc/syslinux.txt b/doc/syslinux.txt
index a4b201f..0dfecee 100644
--- a/doc/syslinux.txt
+++ b/doc/syslinux.txt
@@ -551,7 +551,7 @@ F12 filename
 	<Ctrl-F>0.
 
 PATH path
-	Specify a colon-separated (':') list of directories to search
+	Specify a list of directories (separated by spaces) to search
 	when attempting to load modules. This directive is useful for
 	specifying the directories containing the lib*.c32 library
 	files as other modules may be dependent on these files, but
-- 
1.8.1.4

-- 
Matt Fleming, Intel Open Source Technology Center