* [PATCH] tftp: Normalize slashes in tftp paths
@ 2019-10-31 10:33 Javier Martinez Canillas
2019-10-31 12:05 ` Vladimir 'phcoder' Serbinenko
2019-11-29 15:53 ` Daniel Kiper
0 siblings, 2 replies; 5+ messages in thread
From: Javier Martinez Canillas @ 2019-10-31 10:33 UTC (permalink / raw)
To: grub-devel; +Cc: Lenny Szubowicz, Mark Salter, Javier Martinez Canillas
From: Lenny Szubowicz <lszubowi@redhat.com>
Some tftp servers do not handle multiple consecutive slashes correctly;
this patch avoids sending tftp requests with non-normalized paths.
Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
grub-core/net/tftp.c | 28 +++++++++++++++++++++++++---
1 file changed, 25 insertions(+), 3 deletions(-)
diff --git grub-core/net/tftp.c grub-core/net/tftp.c
index 7d90bf66e76..6dbb9cdbb7a 100644
--- grub-core/net/tftp.c
+++ grub-core/net/tftp.c
@@ -300,6 +300,25 @@ destroy_pq (tftp_data_t data)
grub_priority_queue_destroy (data->pq);
}
+/* Create a normalized copy of the filename.
+ Compress any string of consecutive forward slashes to a single forward
+ slash. */
+static void
+grub_normalize_filename (char *normalized, const char *filename)
+{
+ char *dest = normalized;
+ const char *src = filename;
+
+ while (*src != '\0')
+ {
+ if (src[0] == '/' && src[1] == '/')
+ src++;
+ else
+ *dest++ = *src++;
+ }
+ *dest = '\0';
+}
+
static grub_err_t
tftp_open (struct grub_file *file, const char *filename)
{
@@ -337,9 +356,12 @@ tftp_open (struct grub_file *file, const char *filename)
rrqlen = 0;
tftph->opcode = grub_cpu_to_be16_compile_time (TFTP_RRQ);
- grub_strcpy (rrq, filename);
- rrqlen += grub_strlen (filename) + 1;
- rrq += grub_strlen (filename) + 1;
+
+ /* Copy and normalize the filename to work-around issues on some tftp
+ servers when file names are being matched for remapping. */
+ grub_normalize_filename (rrq, filename);
+ rrqlen += grub_strlen (rrq) + 1;
+ rrq += grub_strlen (rrq) + 1;
grub_strcpy (rrq, "octet");
rrqlen += grub_strlen ("octet") + 1;
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tftp: Normalize slashes in tftp paths
2019-10-31 10:33 [PATCH] tftp: Normalize slashes in tftp paths Javier Martinez Canillas
@ 2019-10-31 12:05 ` Vladimir 'phcoder' Serbinenko
2019-11-06 11:22 ` Daniel Kiper
2019-11-29 15:53 ` Daniel Kiper
1 sibling, 1 reply; 5+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2019-10-31 12:05 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]
Having // at the beginning of the path may have special meaning according
to posix. I don't know if it applies in particular case and if the special
meaning is useful for grub to begin with, just something to check
On Thu, 31 Oct 2019, 11:37 Javier Martinez Canillas, <javierm@redhat.com>
wrote:
> From: Lenny Szubowicz <lszubowi@redhat.com>
>
> Some tftp servers do not handle multiple consecutive slashes correctly;
> this patch avoids sending tftp requests with non-normalized paths.
>
> Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> grub-core/net/tftp.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git grub-core/net/tftp.c grub-core/net/tftp.c
> index 7d90bf66e76..6dbb9cdbb7a 100644
> --- grub-core/net/tftp.c
> +++ grub-core/net/tftp.c
> @@ -300,6 +300,25 @@ destroy_pq (tftp_data_t data)
> grub_priority_queue_destroy (data->pq);
> }
>
> +/* Create a normalized copy of the filename.
> + Compress any string of consecutive forward slashes to a single forward
> + slash. */
> +static void
> +grub_normalize_filename (char *normalized, const char *filename)
> +{
> + char *dest = normalized;
> + const char *src = filename;
> +
> + while (*src != '\0')
> + {
> + if (src[0] == '/' && src[1] == '/')
> + src++;
> + else
> + *dest++ = *src++;
> + }
> + *dest = '\0';
> +}
> +
> static grub_err_t
> tftp_open (struct grub_file *file, const char *filename)
> {
> @@ -337,9 +356,12 @@ tftp_open (struct grub_file *file, const char
> *filename)
> rrqlen = 0;
>
> tftph->opcode = grub_cpu_to_be16_compile_time (TFTP_RRQ);
> - grub_strcpy (rrq, filename);
> - rrqlen += grub_strlen (filename) + 1;
> - rrq += grub_strlen (filename) + 1;
> +
> + /* Copy and normalize the filename to work-around issues on some tftp
> + servers when file names are being matched for remapping. */
> + grub_normalize_filename (rrq, filename);
> + rrqlen += grub_strlen (rrq) + 1;
> + rrq += grub_strlen (rrq) + 1;
>
> grub_strcpy (rrq, "octet");
> rrqlen += grub_strlen ("octet") + 1;
> --
> 2.21.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
[-- Attachment #2: Type: text/html, Size: 3474 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tftp: Normalize slashes in tftp paths
2019-10-31 12:05 ` Vladimir 'phcoder' Serbinenko
@ 2019-11-06 11:22 ` Daniel Kiper
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Kiper @ 2019-11-06 11:22 UTC (permalink / raw)
To: Vladimir 'phcoder' Serbinenko; +Cc: The development of GNU GRUB
On Thu, Oct 31, 2019 at 01:05:09PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Having // at the beginning of the path may have special meaning according
> to posix. I don't know if it applies in particular case and if the special
> meaning is useful for grub to begin with, just something to check
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13
"A pathname consisting of a single <slash> shall resolve to the root
directory of the process. A null pathname shall not be successfully
resolved. If a pathname begins with two successive <slash> characters,
the first component following the leading <slash> characters may be
interpreted in an implementation-defined manner, although more than two
leading <slash> characters shall be treated as a single <slash>
character."
I do not see any reason to have "implementation-defined manner" for "//"
in the GRUB. So, IMO we should replace any set of consecutive slashes
with one slash.
> On Thu, 31 Oct 2019, 11:37 Javier Martinez Canillas, <javierm@redhat.com>
> wrote:
>
> > From: Lenny Szubowicz <lszubowi@redhat.com>
> >
> > Some tftp servers do not handle multiple consecutive slashes correctly;
> > this patch avoids sending tftp requests with non-normalized paths.
> >
> > Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> >
> > grub-core/net/tftp.c | 28 +++++++++++++++++++++++++---
> > 1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git grub-core/net/tftp.c grub-core/net/tftp.c
> > index 7d90bf66e76..6dbb9cdbb7a 100644
> > --- grub-core/net/tftp.c
> > +++ grub-core/net/tftp.c
> > @@ -300,6 +300,25 @@ destroy_pq (tftp_data_t data)
> > grub_priority_queue_destroy (data->pq);
> > }
> >
> > +/* Create a normalized copy of the filename.
> > + Compress any string of consecutive forward slashes to a single forward
> > + slash. */
https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
Please fix this comment...
> > +static void
> > +grub_normalize_filename (char *normalized, const char *filename)
> > +{
> > + char *dest = normalized;
> > + const char *src = filename;
> > +
> > + while (*src != '\0')
> > + {
> > + if (src[0] == '/' && src[1] == '/')
> > + src++;
> > + else
> > + *dest++ = *src++;
> > + }
> > + *dest = '\0';
> > +}
> > +
> > static grub_err_t
> > tftp_open (struct grub_file *file, const char *filename)
> > {
> > @@ -337,9 +356,12 @@ tftp_open (struct grub_file *file, const char
> > *filename)
> > rrqlen = 0;
> >
> > tftph->opcode = grub_cpu_to_be16_compile_time (TFTP_RRQ);
> > - grub_strcpy (rrq, filename);
> > - rrqlen += grub_strlen (filename) + 1;
> > - rrq += grub_strlen (filename) + 1;
> > +
> > + /* Copy and normalize the filename to work-around issues on some tftp
> > + servers when file names are being matched for remapping. */
Ditto.
> > + grub_normalize_filename (rrq, filename);
> > + rrqlen += grub_strlen (rrq) + 1;
> > + rrq += grub_strlen (rrq) + 1;
If you fix these minor issues feel free to add
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
You can also add above excerpt from POSIX spec to the commit message.
This way everybody will know that this is our deliberate decision.
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tftp: Normalize slashes in tftp paths
2019-10-31 10:33 [PATCH] tftp: Normalize slashes in tftp paths Javier Martinez Canillas
2019-10-31 12:05 ` Vladimir 'phcoder' Serbinenko
@ 2019-11-29 15:53 ` Daniel Kiper
2019-12-13 12:59 ` Javier Martinez Canillas
1 sibling, 1 reply; 5+ messages in thread
From: Daniel Kiper @ 2019-11-29 15:53 UTC (permalink / raw)
To: Javier Martinez Canillas; +Cc: grub-devel, Lenny Szubowicz, Mark Salter
On Thu, Oct 31, 2019 at 11:33:39AM +0100, Javier Martinez Canillas wrote:
> From: Lenny Szubowicz <lszubowi@redhat.com>
>
> Some tftp servers do not handle multiple consecutive slashes correctly;
> this patch avoids sending tftp requests with non-normalized paths.
>
> Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Except some nitpicks which I fix before committing...
> ---
>
> grub-core/net/tftp.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git grub-core/net/tftp.c grub-core/net/tftp.c
> index 7d90bf66e76..6dbb9cdbb7a 100644
> --- grub-core/net/tftp.c
> +++ grub-core/net/tftp.c
> @@ -300,6 +300,25 @@ destroy_pq (tftp_data_t data)
> grub_priority_queue_destroy (data->pq);
> }
>
> +/* Create a normalized copy of the filename.
> + Compress any string of consecutive forward slashes to a single forward
> + slash. */
https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
> +static void
> +grub_normalize_filename (char *normalized, const char *filename)
> +{
> + char *dest = normalized;
> + const char *src = filename;
> +
> + while (*src != '\0')
> + {
> + if (src[0] == '/' && src[1] == '/')
> + src++;
> + else
> + *dest++ = *src++;
> + }
> + *dest = '\0';
> +}
> +
> static grub_err_t
> tftp_open (struct grub_file *file, const char *filename)
> {
> @@ -337,9 +356,12 @@ tftp_open (struct grub_file *file, const char *filename)
> rrqlen = 0;
>
> tftph->opcode = grub_cpu_to_be16_compile_time (TFTP_RRQ);
> - grub_strcpy (rrq, filename);
> - rrqlen += grub_strlen (filename) + 1;
> - rrq += grub_strlen (filename) + 1;
> +
> + /* Copy and normalize the filename to work-around issues on some tftp
> + servers when file names are being matched for remapping. */
Ditto.
> + grub_normalize_filename (rrq, filename);
> + rrqlen += grub_strlen (rrq) + 1;
> + rrq += grub_strlen (rrq) + 1;
>
> grub_strcpy (rrq, "octet");
> rrqlen += grub_strlen ("octet") + 1;
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tftp: Normalize slashes in tftp paths
2019-11-29 15:53 ` Daniel Kiper
@ 2019-12-13 12:59 ` Javier Martinez Canillas
0 siblings, 0 replies; 5+ messages in thread
From: Javier Martinez Canillas @ 2019-12-13 12:59 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, Lenny Szubowicz, Mark Salter
Hello Daniel,
On 11/29/19 4:53 PM, Daniel Kiper wrote:
> On Thu, Oct 31, 2019 at 11:33:39AM +0100, Javier Martinez Canillas wrote:
>> From: Lenny Szubowicz <lszubowi@redhat.com>
>>
>> Some tftp servers do not handle multiple consecutive slashes correctly;
>> this patch avoids sending tftp requests with non-normalized paths.
>>
>> Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
>> Signed-off-by: Mark Salter <msalter@redhat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> Except some nitpicks which I fix before committing...
>
Thanks a lot for fixing the issues and taking the patch.
>> ---
>>
>> grub-core/net/tftp.c | 28 +++++++++++++++++++++++++---
>> 1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git grub-core/net/tftp.c grub-core/net/tftp.c
>> index 7d90bf66e76..6dbb9cdbb7a 100644
>> --- grub-core/net/tftp.c
>> +++ grub-core/net/tftp.c
>> @@ -300,6 +300,25 @@ destroy_pq (tftp_data_t data)
>> grub_priority_queue_destroy (data->pq);
>> }
>>
>> +/* Create a normalized copy of the filename.
>> + Compress any string of consecutive forward slashes to a single forward
>> + slash. */
>
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
>
Sorry, I'll make sure that comments are correct the next time.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-13 13:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 10:33 [PATCH] tftp: Normalize slashes in tftp paths Javier Martinez Canillas
2019-10-31 12:05 ` Vladimir 'phcoder' Serbinenko
2019-11-06 11:22 ` Daniel Kiper
2019-11-29 15:53 ` Daniel Kiper
2019-12-13 12:59 ` Javier Martinez Canillas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.