Re: [OpenBIOS] r565 - trunk/openbios-devel/modules

[Laurent Vivier]

Le samedi 22 août 2009 à 08:03 +0200, [e-mail] a écrit :
> Author: blueswirl
> Date: 2009-08-22 08:03:40 +0200 (Sat, 22 Aug 2009)
> New Revision: 565
>
> Modified:
>    trunk/openbios-devel/modules/disk-label.c
> Log:
> Allow NULL dlabel path argument without accessing page zero.
>
> Signed-off-by: Blue Swirl
> Modified: trunk/openbios-devel/modules/disk-label.c
>
> --- trunk/openbios-devel/modules/disk-label.c	2009-08-22 06:03:20 UTC (rev 564)
> +++ trunk/openbios-devel/modules/disk-label.c	2009-08-22 06:03:40 UTC (rev 565)
> @@ -57,7 +57,7 @@
>  static void
>  dlabel_open( dlabel_info_t *di )
>  {
> char *s, *filename;
> const char *s, *filename;
> char *path;
> char block0[512];
> phandle_t ph;
>
> @@ -65,6 +65,9 @@
>    xt_t xt;
>
> path = my_args_copy();
>
> if (!path) {
> path = strdup("");
>
> }
> DPRINTF("dlabel-open '%s'\n", path );
>
> /* open disk interface */

This patch breaks booting PPC using QUIK bootloader. Ìs it possible to
revert it ?

Regards,
Laurent

r565 - trunk/openbios-devel/modules

[Igor Kovalenko]

On Thu, Sep 3, 2009 at 1:15 AM, Laurent Vivier wrote:
> Le samedi 22 août 2009 à 08:03 +0200, [e-mail] a écrit :
> > Author: blueswirl
> > Date: 2009-08-22 08:03:40 +0200 (Sat, 22 Aug 2009)
> > New Revision: 565
> >
> > Modified:
> >    trunk/openbios-devel/modules/disk-label.c
> > Log:
> > Allow NULL dlabel path argument without accessing page zero.
> >
> > Signed-off-by: Blue Swirl
> > Modified: trunk/openbios-devel/modules/disk-label.c
> >
> > --- trunk/openbios-devel/modules/disk-label.c 2009-08-22 06:03:20 UTC (rev 564)
> > +++ trunk/openbios-devel/modules/disk-label.c 2009-08-22 06:03:40 UTC (rev 565)
> > @@ -57,7 +57,7 @@
> >  static void
> >  dlabel_open( dlabel_info_t *di )
> >  {
> > char *s, *filename;
> > const char *s, *filename;
> >
> > char *path;
> >       char block0[512];
> >       phandle_t ph;
> > @@ -65,6 +65,9 @@
> >       xt_t xt;
> >
> > path = my_args_copy();
> > if (!path) {
> > path = strdup("");
> > }
> >
> > DPRINTF("dlabel-open '%s'\n", path );
> >
> > /* open disk interface */
>
> This patch breaks booting PPC using QUIK bootloader. Ìs it possible to
> revert it ?

Can you please check what is the real issue here?
Maybe dlabel_open is getting called somewhere in bootloader
without proper path argument?

r565 - trunk/openbios-devel/modules

[Laurent Vivier]

Le jeudi 03 septembre 2009 à 01:32 +0400, Igor Kovalenko a écrit :
> On Thu, Sep 3, 2009 at 1:15 AM, Laurent Vivier wrote:
> > Le samedi 22 août 2009 à 08:03 +0200, [e-mail] a écrit :
> > > Author: blueswirl
> > > Date: 2009-08-22 08:03:40 +0200 (Sat, 22 Aug 2009)
> > > New Revision: 565
> > >
> > > Modified:
> > >    trunk/openbios-devel/modules/disk-label.c
> > > Log:
> > > Allow NULL dlabel path argument without accessing page zero.

[...]
> > This patch breaks booting PPC using QUIK bootloader. Ìs it possible to
> > revert it ?
>
> Can you please check what is the real issue here?

Yes, but my first idea was to make it done by someone else... ;-)
> Maybe dlabel_open is getting called somewhere in bootloader
> without proper path argument?

In fact, patch r565 modifies the behavior of dlabel_open() because
filename is not NULL anymore if path is empty.

This new patch corrects the problem:

Index: modules/disk-label.c
===================================================================
--- modules/disk-label.c	(révision 576)
+++ modules/disk-label.c	(copie de travail)
@@ -146,7 +146,8 @@
    	push_str( filename );
    	PUSH_ph( ph );
    	fword("interpose");
-	} else if (filename && strcmp(filename, "%BOOT") != 0) {
+	} else if (filename && *filename != 0 &&
+		   strcmp(filename, "%BOOT") != 0) {
    	goto out;
    }
    success = 1;

You can comment, I'll apply it later...

Regards,
Laurent

r565 - trunk/openbios-devel/modules

[Igor Kovalenko]

On Thu, Sep 3, 2009 at 1:54 AM, Laurent Vivier wrote:
> Le jeudi 03 septembre 2009 à 01:32 +0400, Igor Kovalenko a écrit :
> > On Thu, Sep 3, 2009 at 1:15 AM, Laurent Vivier wrote:
> > > Le samedi 22 août 2009 à 08:03 +0200, [e-mail] a écrit :
> > > > Author: blueswirl
> > > > Date: 2009-08-22 08:03:40 +0200 (Sat, 22 Aug 2009)
> > > > New Revision: 565
> > > >
> > > > Modified:
> > > >    trunk/openbios-devel/modules/disk-label.c
> > > > Log:
> > > > Allow NULL dlabel path argument without accessing page zero.
>
> [...]
> > > This patch breaks booting PPC using QUIK bootloader. Ìs it possible to
> > > revert it ?
> >
> > Can you please check what is the real issue here?
>
> Yes, but my first idea was to make it done by someone else... ;-)
> > Maybe dlabel_open is getting called somewhere in bootloader
> > without proper path argument?
>
> In fact, patch r565 modifies the behavior of dlabel_open() because
> filename is not NULL anymore if path is empty.
>
> This new patch corrects the problem:
> Index: modules/disk-label.c
>
> --- modules/disk-label.c        (révision 576)
> +++ modules/disk-label.c        (copie de travail)
> @@ -146,7 +146,8 @@
>                push_str( filename );
>                PUSH_ph( ph );
>                fword("interpose");
> } else if (filename && strcmp(filename, "%BOOT") != 0) {
> } else if (filename && *filename != 0 &&
> strcmp(filename, "%BOOT") != 0) {
>
> goto out;
>        }
>        success = 1;
>
> You can comment, I'll apply it later...

They say, good catch :)
dlabel_open also assumes *(0)==0 which is not the case generally,
so let's fix dlabel_open instead of reverting r565

You may want to initialize filename="" as well, and remove check
for NULL filename so only strcmp is left.

r565 - trunk/openbios-devel/modules

[Laurent Vivier]

Le jeudi 03 septembre 2009 à 02:08 +0400, Igor Kovalenko a écrit :
> On Thu, Sep 3, 2009 at 1:54 AM, Laurent Vivier wrote:
> > Le jeudi 03 septembre 2009 à 01:32 +0400, Igor Kovalenko a écrit :
> > > On Thu, Sep 3, 2009 at 1:15 AM, Laurent Vivier wrote:
> > > > Le samedi 22 août 2009 à 08:03 +0200, [e-mail] a écrit :
> > > > > Author: blueswirl
> > > > > Date: 2009-08-22 08:03:40 +0200 (Sat, 22 Aug 2009)
> > > > > New Revision: 565
> > > > >
> > > > > Modified:
> > > > >    trunk/openbios-devel/modules/disk-label.c
> > > > > Log:
> > > > > Allow NULL dlabel path argument without accessing page zero.
> >
> > [...]
> > > > This patch breaks booting PPC using QUIK bootloader. Ìs it possible to
> > > > revert it ?
> > >
> > > Can you please check what is the real issue here?
> >
> > Yes, but my first idea was to make it done by someone else... ;-)
> > > Maybe dlabel_open is getting called somewhere in bootloader
> > > without proper path argument?
> >
> > In fact, patch r565 modifies the behavior of dlabel_open() because
> > filename is not NULL anymore if path is empty.
> >
> > This new patch corrects the problem:
> > Index: modules/disk-label.c
> >
> > --- modules/disk-label.c        (révision 576)
> > +++ modules/disk-label.c        (copie de travail)
> > @@ -146,7 +146,8 @@
> >                push_str( filename );
> >                PUSH_ph( ph );
> >                fword("interpose");
> >   } else if (filename && strcmp(filename, "%BOOT") != 0) {
> >   } else if (filename && *filename != 0 &&
> >              strcmp(filename, "%BOOT") != 0) {
> >          goto out;
> >  }
> >  success = 1;
> >
> > You can comment, I'll apply it later...
>
> They say, good catch :)
> dlabel_open also assumes *(0)==0 which is not the case generally,
> so let's fix dlabel_open instead of reverting r565
>
> You may want to initialize filename="" as well, and remove check
> for NULL filename so only strcmp is left.

Good idea.

Laurent