[PATCH v1 0/2] Fix cfg file parsing and command line parsing

[PATCH v1 0/2] Fix cfg file parsing and command line parsing

[Shreenidhi Shedi]

Hi GRUB Developers,

This patch series contains two fixes to improve how GRUB handles
environment files and constructs the kernel command line, these issues
were reported by PhotonOS customers.

The first patch ensures that empty lines in environment files are
skipped during parsing.

The second patch removes unnecessary escaping of quotes in the
constructed command line, as the Linux kernel does not support escaped
quotes.

Thanks for your time and consideration.

v0 changes:
- Initial revision.
- https://lists.gnu.org/archive/html/grub-devel/2025-05/msg00085.html

v1 changes:
- Added cover leter
- Improved commit message as suggested by Daniel Kiper

--
Shreenidhi Shedi

--
2.49.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v1 1/2] envblk: ignore empty new lines while parsing env files

[Shreenidhi Shedi]

From: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>

Environment files may contain empty lines, which should be
ignored during parsing. Currently, these lines are not skipped and
resulting in incorrect behavior. This patch adds a check to skip empty
lines along with those starting with `#'.

Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
Reviewed-by: Alexey Makhalov  <alexey.makhalov@broadcom.com>
---
 grub-core/lib/envblk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
index 2e4e78b13..f08dabaad 100644
--- a/grub-core/lib/envblk.c
+++ b/grub-core/lib/envblk.c
@@ -235,7 +235,7 @@ grub_envblk_iterate (grub_envblk_t envblk,

   while (p < pend)
     {
-      if (*p != '#')
+      if (*p != '#' && *p != '\n')
         {
           char *name;
           char *value;
--
2.49.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v1 1/2] envblk: ignore empty new lines while parsing env files

[Daniel Kiper]

On Sun, May 18, 2025 at 01:21:20PM +0530, Shreenidhi Shedi wrote:
> From: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
>
> Environment files may contain empty lines, which should be
> ignored during parsing. Currently, these lines are not skipped and
> resulting in incorrect behavior. This patch adds a check to skip empty
> lines along with those starting with `#'.
>
> Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
> Reviewed-by: Alexey Makhalov  <alexey.makhalov@broadcom.com>

Please do not add RBs if reviews have not been done in public.
Those given on the grub-devel counts only.

> ---
>  grub-core/lib/envblk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
> index 2e4e78b13..f08dabaad 100644
> --- a/grub-core/lib/envblk.c
> +++ b/grub-core/lib/envblk.c
> @@ -235,7 +235,7 @@ grub_envblk_iterate (grub_envblk_t envblk,
>
>    while (p < pend)
>      {
> -      if (*p != '#')
> +      if (*p != '#' && *p != '\n')

Why do not add '\r' too?

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v1 1/2] envblk: ignore empty new lines while parsing env files

[Alexey Makhalov via Grub-devel]

On 5/19/25 10:13 AM, Daniel Kiper wrote:
> On Sun, May 18, 2025 at 01:21:20PM +0530, Shreenidhi Shedi wrote:
>> From: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
>>
>> Environment files may contain empty lines, which should be
>> ignored during parsing. Currently, these lines are not skipped and
>> resulting in incorrect behavior. This patch adds a check to skip empty
>> lines along with those starting with `#'.
>>
>> Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
>> Reviewed-by: Alexey Makhalov  <alexey.makhalov@broadcom.com>
> 
> Please do not add RBs if reviews have not been done in public.
> Those given on the grub-devel counts only.
> 

Hi Daniel, it was my ask to Shreenidhi to add BR after our internal review.
Noted that part for the future.

Thanks,
--Alexey

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v1 1/2] envblk: ignore empty new lines while parsing env files

[Shreenidhi Shedi]

On 19/05/25 22:43, Daniel Kiper wrote:
> On Sun, May 18, 2025 at 01:21:20PM +0530, Shreenidhi Shedi wrote:
>> From: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
>>
>> Environment files may contain empty lines, which should be
>> ignored during parsing. Currently, these lines are not skipped and
>> resulting in incorrect behavior. This patch adds a check to skip empty
>> lines along with those starting with `#'.
>>
>> Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
>> Reviewed-by: Alexey Makhalov  <alexey.makhalov@broadcom.com>
> 
> Please do not add RBs if reviews have not been done in public.
> Those given on the grub-devel counts only.
> 
>> ---
>>   grub-core/lib/envblk.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
>> index 2e4e78b13..f08dabaad 100644
>> --- a/grub-core/lib/envblk.c
>> +++ b/grub-core/lib/envblk.c
>> @@ -235,7 +235,7 @@ grub_envblk_iterate (grub_envblk_t envblk,
>>
>>     while (p < pend)
>>       {
>> -      if (*p != '#')
>> +      if (*p != '#' && *p != '\n')
> 
> Why do not add '\r' too?
> 
> Daniel

On suggestion regarding RBs, will keep it in mind going forward.

I will add check to ignore '\r' and send v2 shortly.

Thanks for the review.

-- 
Shedi

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v1 1/2] envblk: ignore empty new lines while parsing env files

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 1292 bytes --]

Are those empty lines result of manual editing of envblk? You should not do
this

Regards

Le dim. 18 mai 2025, 11:01, Shreenidhi Shedi <yesshedi@gmail.com> a écrit :

> From: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
>
> Environment files may contain empty lines, which should be
> ignored during parsing. Currently, these lines are not skipped and
> resulting in incorrect behavior. This patch adds a check to skip empty
> lines along with those starting with `#'.
>
> Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
> Reviewed-by: Alexey Makhalov  <alexey.makhalov@broadcom.com>
> ---
>  grub-core/lib/envblk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
> index 2e4e78b13..f08dabaad 100644
> --- a/grub-core/lib/envblk.c
> +++ b/grub-core/lib/envblk.c
> @@ -235,7 +235,7 @@ grub_envblk_iterate (grub_envblk_t envblk,
>
>    while (p < pend)
>      {
> -      if (*p != '#')
> +      if (*p != '#' && *p != '\n')
>          {
>            char *name;
>            char *value;
> --
> 2.49.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2315 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v1 2/2] cmdline: fix command line parsing in grub

[Shreenidhi Shedi]

From: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>

Previously, the command line construction function
(grub_create_loader_cmdline) escaped single and double quotes, which is
unnecessary and potentially problematic since the kernel command line
handler does not support escaped quotes. This patch removes the escaping
of these characters, ensuring that the constructed command line is
passed to the kernel as intended.

For example:

In /boot/grub/grub.cfg
...
set user_cmdline="dyndbg='+p; func smp_callin -p'"
...
menuentry "Photon" {
  linux /boot/$photon_linux root=$rootpartition $photon_cmdline $systemd_cmdline $user_cmdline
  if [ -f /boot/$photon_initrd ]; then
    initrd /boot/$photon_initrd
  fi
}
...

Post reboot,

$ cat /proc/cmdline
... dyndbg=\'+p; func smp_callin -p\'

Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
Reviewed-by: Alexey Makhalov  <alexey.makhalov@broadcom.com>
---
 grub-core/lib/cmdline.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/grub-core/lib/cmdline.c b/grub-core/lib/cmdline.c
index ed0b149dc..57c96a151 100644
--- a/grub-core/lib/cmdline.c
+++ b/grub-core/lib/cmdline.c
@@ -22,7 +22,6 @@

 static unsigned int check_arg (char *c, int *has_space)
 {
-  int space = 0;
   unsigned int size = 0;

   while (*c)
@@ -51,15 +50,9 @@ unsigned int grub_loader_cmdline_size (int argc, char *argv[])
   unsigned int size = 0;

   for (i = 0; i < argc; i++)
-    {
       size += check_arg (argv[i], 0);
-      size++; /* Separator space or NULL.  */
-    }
-
-  if (size == 0)
-    size = 1;

-  return size;
+  return size ? size : 1;
 }

 grub_err_t
@@ -73,8 +66,8 @@ grub_create_loader_cmdline (int argc, char *argv[], char *buf,
   for (i = 0; i < argc; i++)
     {
       c = argv[i];
+      space = 0;
       arg_size = check_arg(argv[i], &space);
-      arg_size++; /* Separator space or NULL.  */

       if (size < arg_size)
 	break;
@@ -86,11 +79,7 @@ grub_create_loader_cmdline (int argc, char *argv[], char *buf,

       while (*c)
 	{
-	  if (*c == '\\' || *c == '\'' || *c == '"')
-	    *buf++ = '\\';
-
-	  *buf++ = *c;
-	  c++;
+	  *buf++ = *c++;
 	}

       if (space)
--
2.49.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel