diff mbox series

[v2,3/3] discover/grub: Improve BLS grub environment variables expansion

Message ID 20180607173535.20629-4-javierm@redhat.com
State Changes Requested
Headers show
Series discover/grub: Some improvements and fixes for BLS support | expand

Commit Message

Javier Martinez Canillas June 7, 2018, 5:35 p.m. UTC 
The fields from a BootLoaderSpec file can contain environment variables,
in GRUB 2 these are show verbatim and are evaluated later when an entry
is selected. But on Petitboot these have to be expanded before creating
the GRUB 2 resources and show in the UI the values after the evaluation.

The current blscfg handler had a very limited support for variables, it
only had support for the options field and also didn't take into account
that variables could be mixed with literal values.

So for example the following fields were not expanded correctly:

  linux   $bootprefix/vmlinuz

  options $kernelopts foo=bar

  options foo=bar $kernelopts

  options $kernelopts $debugopts

Also change some of the tests to cover mixing variables and literals.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

 discover/grub2/blscfg.c                      | 86 ++++++++++++++++----
 test/parser/test-grub2-blscfg-multiple-bls.c |  5 +-
 test/parser/test-grub2-blscfg-opts-grubenv.c |  4 +-
 3 files changed, 75 insertions(+), 20 deletions(-)

Comments

Sam Mendoza-Jonas June 12, 2018, 4:32 a.m. UTC | #1 
On Thu, 2018-06-07 at 19:35 +0200, Javier Martinez Canillas wrote:
> The fields from a BootLoaderSpec file can contain environment variables,
> in GRUB 2 these are show verbatim and are evaluated later when an entry
> is selected. But on Petitboot these have to be expanded before creating
> the GRUB 2 resources and show in the UI the values after the evaluation.
> 
> The current blscfg handler had a very limited support for variables, it
> only had support for the options field and also didn't take into account
> that variables could be mixed with literal values.
> 
> So for example the following fields were not expanded correctly:
> 
>   linux   $bootprefix/vmlinuz
> 
>   options $kernelopts foo=bar
> 
>   options foo=bar $kernelopts
> 
>   options $kernelopts $debugopts
> 
> Also change some of the tests to cover mixing variables and literals.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> ---
> 
>  discover/grub2/blscfg.c                      | 86 ++++++++++++++++----
>  test/parser/test-grub2-blscfg-multiple-bls.c |  5 +-
>  test/parser/test-grub2-blscfg-opts-grubenv.c |  4 +-
>  3 files changed, 75 insertions(+), 20 deletions(-)
> 
> diff --git a/discover/grub2/blscfg.c b/discover/grub2/blscfg.c
> index a3813064a0a..fdbe2468952 100644
> --- a/discover/grub2/blscfg.c
> +++ b/discover/grub2/blscfg.c
> @@ -2,6 +2,7 @@
>  #define _GNU_SOURCE
>  
>  #include <assert.h>
> +#include <ctype.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <dirent.h>
> @@ -34,54 +35,107 @@ struct bls_state {
>  	const char *dtb;
>  };
>  
> +static char *field_append(struct bls_state *state, int type, char *buffer,
> +			  char *start, char *end)
> +{
> +	char *temp = talloc_strndup(state, start, end - start + 1);
> +	temp[end - start + 1] = '\0';

This can go one byte past the end of temp depending on what start and end
are; _strndup() should terminate this buffer itself anyway.

> +	const char *field = temp;

Also mostly for looks try not to mix declarations and code :)

Cheers,
Sam

> +
> +	if (type == GRUB2_WORD_VAR) {
> +		field = script_env_get(state->script, temp);
> +		if (!field)
> +			return buffer;
> +	}
> +
> +	if (!buffer)
> +		buffer = talloc_strdup(state->opt, field);
> +	else
> +		buffer = talloc_asprintf_append(buffer, "%s", field);
> +
> +	return buffer;
> +}
> +
> +static char *expand_field(struct bls_state *state, char *value)
> +{
> +	char *buffer = NULL;
> +	char *start = value;
> +	char *end = value;
> +	int type = GRUB2_WORD_TEXT;
> +
> +	while (*value) {
> +		if (*value == '$') {
> +			if (start != end) {
> +				buffer = field_append(state, type, buffer,
> +						      start, end);
> +				if (!buffer)
> +					return NULL;
> +			}
> +
> +			type = GRUB2_WORD_VAR;
> +			start = value + 1;
> +		} else if (type == GRUB2_WORD_VAR) {
> +			if (!isalnum(*value) && *value != '_') {
> +				buffer = field_append(state, type, buffer,
> +						      start, end);
> +				type = GRUB2_WORD_TEXT;
> +				start = value;
> +			}
> +		}
> +
> +		end = value;
> +		value++;
> +	}
> +
> +	if (start != end) {
> +		buffer = field_append(state, type, buffer,
> +				      start, end);
> +		if (!buffer)
> +			return NULL;
> +	}
> +
> +	return buffer;
> +}
> +
>  static void bls_process_pair(struct conf_context *conf, const char *name,
>  			     char *value)
>  {
>  	struct bls_state *state = conf->parser_info;
>  	struct discover_boot_option *opt = state->opt;
>  	struct boot_option *option = opt->option;
> -	const char *boot_args;
>  
>  	if (streq(name, "title")) {
> -		state->title = talloc_strdup(state, value);
> +		state->title = expand_field(state, value);
>  		return;
>  	}
>  
>  	if (streq(name, "version")) {
> -		state->version = talloc_strdup(state, value);
> +		state->version = expand_field(state, value);
>  		return;
>  	}
>  
>  	if (streq(name, "machine-id")) {
> -		state->machine_id = talloc_strdup(state, value);
> +		state->machine_id = expand_field(state, value);
>  		return;
>  	}
>  
>  	if (streq(name, "linux")) {
> -		state->image = talloc_strdup(state, value);
> +		state->image = expand_field(state, value);
>  		return;
>  	}
>  
>  	if (streq(name, "initrd")) {
> -		state->initrd = talloc_strdup(state, value);
> +		state->initrd = expand_field(state, value);
>  		return;
>  	}
>  
>  	if (streq(name, "devicetree")) {
> -		state->dtb = talloc_strdup(state, value);
> +		state->dtb = expand_field(state, value);
>  		return;
>  	}
>  
>  	if (streq(name, "options")) {
> -		if (value[0] == '$') {
> -			boot_args = script_env_get(state->script, value + 1);
> -			if (!boot_args)
> -				return;
> -
> -			option->boot_args = talloc_strdup(opt, boot_args);
> -		} else {
> -			option->boot_args = talloc_strdup(opt, value);
> -		}
> +		option->boot_args = expand_field(state, value);
>  		return;
>  	}
>  }
> diff --git a/test/parser/test-grub2-blscfg-multiple-bls.c b/test/parser/test-grub2-blscfg-multiple-bls.c
> index 94f40d191fa..d15fb24fd7e 100644
> --- a/test/parser/test-grub2-blscfg-multiple-bls.c
> +++ b/test/parser/test-grub2-blscfg-multiple-bls.c
> @@ -1,6 +1,7 @@
>  #include "parser-test.h"
>  
>  #if 0 /* PARSER_EMBEDDED_CONFIG */
> +set os_name=Fedora
>  blscfg
>  #endif
>  
> @@ -13,14 +14,14 @@ void run_test(struct parser_test *test)
>  
>  	test_add_file_string(test, test->ctx->device,
>  			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",
> -			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "title $os_name (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
>  			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
>  			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
>  			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n\n");
>  
>  	test_add_file_string(test, test->ctx->device,
>  			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.14.18-300.fc28.x86_64.conf",
> -			     "title Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "title $os_name (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)\n"
>  			     "linux /vmlinuz-4.14.18-300.fc28.x86_64\n"
>  			     "initrd /initramfs-4.14.18-300.fc28.x86_64.img\n"
>  			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n");
> diff --git a/test/parser/test-grub2-blscfg-opts-grubenv.c b/test/parser/test-grub2-blscfg-opts-grubenv.c
> index 544a5de4d23..2dffd1b730c 100644
> --- a/test/parser/test-grub2-blscfg-opts-grubenv.c
> +++ b/test/parser/test-grub2-blscfg-opts-grubenv.c
> @@ -22,7 +22,7 @@ void run_test(struct parser_test *test)
>  			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
>  			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
>  			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
> -			     "options $kernelopts\n");
> +			     "options $kernelopts debug\n");
>  
>  	test_read_conf_embedded(test, "/boot/grub2/grub.cfg");
>  
> @@ -32,5 +32,5 @@ void run_test(struct parser_test *test)
>  
>  	opt = get_boot_option(ctx, 0);
>  
> -	check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root");
> +	check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root debug");
>  }
Javier Martinez Canillas June 12, 2018, 10:55 a.m. UTC | #2 
On 06/12/2018 06:32 AM, Samuel Mendoza-Jonas wrote:
> On Thu, 2018-06-07 at 19:35 +0200, Javier Martinez Canillas wrote:
>> The fields from a BootLoaderSpec file can contain environment variables,
>> in GRUB 2 these are show verbatim and are evaluated later when an entry
>> is selected. But on Petitboot these have to be expanded before creating
>> the GRUB 2 resources and show in the UI the values after the evaluation.
>>
>> The current blscfg handler had a very limited support for variables, it
>> only had support for the options field and also didn't take into account
>> that variables could be mixed with literal values.
>>
>> So for example the following fields were not expanded correctly:
>>
>>   linux   $bootprefix/vmlinuz
>>
>>   options $kernelopts foo=bar
>>
>>   options foo=bar $kernelopts
>>
>>   options $kernelopts $debugopts
>>
>> Also change some of the tests to cover mixing variables and literals.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> ---
>>
>>  discover/grub2/blscfg.c                      | 86 ++++++++++++++++----
>>  test/parser/test-grub2-blscfg-multiple-bls.c |  5 +-
>>  test/parser/test-grub2-blscfg-opts-grubenv.c |  4 +-
>>  3 files changed, 75 insertions(+), 20 deletions(-)
>>
>> diff --git a/discover/grub2/blscfg.c b/discover/grub2/blscfg.c
>> index a3813064a0a..fdbe2468952 100644
>> --- a/discover/grub2/blscfg.c
>> +++ b/discover/grub2/blscfg.c
>> @@ -2,6 +2,7 @@
>>  #define _GNU_SOURCE
>>  
>>  #include <assert.h>
>> +#include <ctype.h>
>>  #include <stdlib.h>
>>  #include <string.h>
>>  #include <dirent.h>
>> @@ -34,54 +35,107 @@ struct bls_state {
>>  	const char *dtb;
>>  };
>>  
>> +static char *field_append(struct bls_state *state, int type, char *buffer,
>> +			  char *start, char *end)
>> +{
>> +	char *temp = talloc_strndup(state, start, end - start + 1);
>> +	temp[end - start + 1] = '\0';
> 
> This can go one byte past the end of temp depending on what start and end
> are; _strndup() should terminate this buffer itself anyway.
>

You are right, I removed this on v3.
 
>> +	const char *field = temp;
> 
> Also mostly for looks try not to mix declarations and code :)
>

Indeed, my bad. This got fixed too by removing the line mentioned above.
 
> Cheers,
> Sam
> 
Best regards,
diff mbox series

Patch

diff --git a/discover/grub2/blscfg.c b/discover/grub2/blscfg.c
index a3813064a0a..fdbe2468952 100644
--- a/discover/grub2/blscfg.c
+++ b/discover/grub2/blscfg.c
@@ -2,6 +2,7 @@ 
 #define _GNU_SOURCE
 
 #include <assert.h>
+#include <ctype.h>
 #include <stdlib.h>
 #include <string.h>
 #include <dirent.h>
@@ -34,54 +35,107 @@  struct bls_state {
 	const char *dtb;
 };
 
+static char *field_append(struct bls_state *state, int type, char *buffer,
+			  char *start, char *end)
+{
+	char *temp = talloc_strndup(state, start, end - start + 1);
+	temp[end - start + 1] = '\0';
+	const char *field = temp;
+
+	if (type == GRUB2_WORD_VAR) {
+		field = script_env_get(state->script, temp);
+		if (!field)
+			return buffer;
+	}
+
+	if (!buffer)
+		buffer = talloc_strdup(state->opt, field);
+	else
+		buffer = talloc_asprintf_append(buffer, "%s", field);
+
+	return buffer;
+}
+
+static char *expand_field(struct bls_state *state, char *value)
+{
+	char *buffer = NULL;
+	char *start = value;
+	char *end = value;
+	int type = GRUB2_WORD_TEXT;
+
+	while (*value) {
+		if (*value == '$') {
+			if (start != end) {
+				buffer = field_append(state, type, buffer,
+						      start, end);
+				if (!buffer)
+					return NULL;
+			}
+
+			type = GRUB2_WORD_VAR;
+			start = value + 1;
+		} else if (type == GRUB2_WORD_VAR) {
+			if (!isalnum(*value) && *value != '_') {
+				buffer = field_append(state, type, buffer,
+						      start, end);
+				type = GRUB2_WORD_TEXT;
+				start = value;
+			}
+		}
+
+		end = value;
+		value++;
+	}
+
+	if (start != end) {
+		buffer = field_append(state, type, buffer,
+				      start, end);
+		if (!buffer)
+			return NULL;
+	}
+
+	return buffer;
+}
+
 static void bls_process_pair(struct conf_context *conf, const char *name,
 			     char *value)
 {
 	struct bls_state *state = conf->parser_info;
 	struct discover_boot_option *opt = state->opt;
 	struct boot_option *option = opt->option;
-	const char *boot_args;
 
 	if (streq(name, "title")) {
-		state->title = talloc_strdup(state, value);
+		state->title = expand_field(state, value);
 		return;
 	}
 
 	if (streq(name, "version")) {
-		state->version = talloc_strdup(state, value);
+		state->version = expand_field(state, value);
 		return;
 	}
 
 	if (streq(name, "machine-id")) {
-		state->machine_id = talloc_strdup(state, value);
+		state->machine_id = expand_field(state, value);
 		return;
 	}
 
 	if (streq(name, "linux")) {
-		state->image = talloc_strdup(state, value);
+		state->image = expand_field(state, value);
 		return;
 	}
 
 	if (streq(name, "initrd")) {
-		state->initrd = talloc_strdup(state, value);
+		state->initrd = expand_field(state, value);
 		return;
 	}
 
 	if (streq(name, "devicetree")) {
-		state->dtb = talloc_strdup(state, value);
+		state->dtb = expand_field(state, value);
 		return;
 	}
 
 	if (streq(name, "options")) {
-		if (value[0] == '$') {
-			boot_args = script_env_get(state->script, value + 1);
-			if (!boot_args)
-				return;
-
-			option->boot_args = talloc_strdup(opt, boot_args);
-		} else {
-			option->boot_args = talloc_strdup(opt, value);
-		}
+		option->boot_args = expand_field(state, value);
 		return;
 	}
 }
diff --git a/test/parser/test-grub2-blscfg-multiple-bls.c b/test/parser/test-grub2-blscfg-multiple-bls.c
index 94f40d191fa..d15fb24fd7e 100644
--- a/test/parser/test-grub2-blscfg-multiple-bls.c
+++ b/test/parser/test-grub2-blscfg-multiple-bls.c
@@ -1,6 +1,7 @@ 
 #include "parser-test.h"
 
 #if 0 /* PARSER_EMBEDDED_CONFIG */
+set os_name=Fedora
 blscfg
 #endif
 
@@ -13,14 +14,14 @@  void run_test(struct parser_test *test)
 
 	test_add_file_string(test, test->ctx->device,
 			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",
-			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
+			     "title $os_name (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
 			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
 			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
 			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n\n");
 
 	test_add_file_string(test, test->ctx->device,
 			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.14.18-300.fc28.x86_64.conf",
-			     "title Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)\n"
+			     "title $os_name (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)\n"
 			     "linux /vmlinuz-4.14.18-300.fc28.x86_64\n"
 			     "initrd /initramfs-4.14.18-300.fc28.x86_64.img\n"
 			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n");
diff --git a/test/parser/test-grub2-blscfg-opts-grubenv.c b/test/parser/test-grub2-blscfg-opts-grubenv.c
index 544a5de4d23..2dffd1b730c 100644
--- a/test/parser/test-grub2-blscfg-opts-grubenv.c
+++ b/test/parser/test-grub2-blscfg-opts-grubenv.c
@@ -22,7 +22,7 @@  void run_test(struct parser_test *test)
 			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
 			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
 			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
-			     "options $kernelopts\n");
+			     "options $kernelopts debug\n");
 
 	test_read_conf_embedded(test, "/boot/grub2/grub.cfg");
 
@@ -32,5 +32,5 @@  void run_test(struct parser_test *test)
 
 	opt = get_boot_option(ctx, 0);
 
-	check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root");
+	check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root debug");
 }