Re: [PATCH] tftp: Normalize slashes in tftp paths

[Daniel Kiper]

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, <address@hidden>
> wrote:
>
> > From: Lenny Szubowicz <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Mark Salter <address@hidden>
> > Signed-off-by: Javier Martinez Canillas <address@hidden>
> > ---
> >
> >  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 <address@hidden>

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