thr3ads.net - Xen devel - [PATCH 0 of 3 RESEND] tools: Move bootloaders to libexec directory [May 2012]

If this information is useful, please help other people find it:
Share via:

George Dunlap

2012-May-09 10:51 UTC

[PATCH 0 of 3 RESEND] tools: Move bootloaders to libexec directory

pygrub and xenpvnetboot are meant to be run by tools, and not by the user,
and thus should be in /usr/lib/xen/bin rather than /usr/bin.  This patch
series:
* Causes libxl to look in the libexec dir if a full path is not specified
* Moves pygrub and xenpvnetboot into the libexec dir
* For compatibility with existing configs, puts a link to pygrub in /usr/bin
* Warns the user that /usr/bin/pygrub is deprecated

George Dunlap

2012-May-09 10:51 UTC

head link

[PATCH 1 of 3 RESEND] libxl: Look for bootloader in libexec path

If the full path for a bootloader (such as pygrub or xenpvnetboot) is not
given, look for it in the libexec path.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 8a86d841e6d4 -r e43157a86933 tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Tue May 08 13:36:24 2012 +0200
+++ b/tools/libxl/libxl_bootloader.c	Wed May 09 11:38:50 2012 +0100
@@ -333,6 +333,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
 
     char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
     char *tempdir;
+    const char *bootloader = NULL;
 
     char *dom_console_xs_path;
     char dom_console_slave_tty_path[PATH_MAX];
@@ -397,6 +398,13 @@ int libxl_run_bootloader(libxl_ctx *ctx,
         goto out_close;
     }
 
+    bootloader = libxl__abs_path(gc, info->u.pv.bootloader,
+                                 libxl__libexec_path());
+    if ( bootloader == NULL ) {
+        rc = ERROR_NOMEM;
+        goto out_close;
+    }
+
     /*
      * We need to present the bootloader''s tty as a pty slave that
xenconsole
      * can access.  Since the bootloader itself needs a pty slave,
@@ -417,7 +425,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
     dom_console_xs_path = libxl__sprintf(gc, "%s/console/tty",
libxl__xs_get_dompath(gc, domid));
     libxl__xs_write(gc, XBT_NULL, dom_console_xs_path, "%s",
dom_console_slave_tty_path);
 
-    pid = fork_exec_bootloader(&bootloader_fd, info->u.pv.bootloader,
args);
+    pid = fork_exec_bootloader(&bootloader_fd, bootloader, args);
     if (pid < 0) {
         goto out_close;
     }

George Dunlap

2012-May-09 10:51 UTC

head link

[PATCH 2 of 3 RESEND] tools: Install pv bootloaders in libexec rather than /usr/bin

pygrub and xenpvnetboot are meant to be run by tools, and not by the user,
and thus should be in /usr/lib/xen/bin rather than /usr/bin.

Because most config files will still have an absolute path pointing to
/usr/bin/pygrub, make a symbolic link that we will deprecate.

diff -r e43157a86933 -r 52f45b258ccc tools/misc/Makefile
--- a/tools/misc/Makefile	Wed May 09 11:38:50 2012 +0100
+++ b/tools/misc/Makefile	Wed May 09 11:39:43 2012 +0100
@@ -18,7 +18,7 @@ SUBDIRS-$(CONFIG_LOMOUNT) += lomount
 SUBDIRS-$(CONFIG_MINITERM) += miniterm
 SUBDIRS := $(SUBDIRS-y)
 
-INSTALL_BIN-y := xencons xenpvnetboot
+INSTALL_BIN-y := xencons
 INSTALL_BIN-$(CONFIG_X86) += xen-detect
 INSTALL_BIN := $(INSTALL_BIN-y)
 
@@ -27,6 +27,9 @@ INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx
 INSTALL_SBIN-$(CONFIG_MIGRATE) += xen-hptool
 INSTALL_SBIN := $(INSTALL_SBIN-y)
 
+INSTALL_PRIVBIN-y := xenpvnetboot
+INSTALL_PRIVBIN := $(INSTALL_PRIVBIN-y)
+
 # Include configure output (config.h) to headers search path
 CFLAGS += -I$(XEN_ROOT)/tools
 
@@ -41,8 +44,10 @@ build: $(TARGETS)
 install: build
 	$(INSTALL_DIR) $(DESTDIR)$(BINDIR)
 	$(INSTALL_DIR) $(DESTDIR)$(SBINDIR)
+	$(INSTALL_DIR) $(DESTDIR)$(PRIVATE_BINDIR)
 	$(INSTALL_PYTHON_PROG) $(INSTALL_BIN) $(DESTDIR)$(BINDIR)
 	$(INSTALL_PYTHON_PROG) $(INSTALL_SBIN) $(DESTDIR)$(SBINDIR)
+	$(INSTALL_PYTHON_PROG) $(INSTALL_PRIVBIN) $(DESTDIR)$(PRIVATE_BINDIR)
 	set -e; for d in $(SUBDIRS); do $(MAKE) -C $$d install-recurse; done
 
 .PHONY: clean
diff -r e43157a86933 -r 52f45b258ccc tools/pygrub/Makefile
--- a/tools/pygrub/Makefile	Wed May 09 11:38:50 2012 +0100
+++ b/tools/pygrub/Makefile	Wed May 09 11:39:43 2012 +0100
@@ -11,9 +11,11 @@ build:
 .PHONY: install
 install: all
 	CC="$(CC)" CFLAGS="$(CFLAGS)" $(PYTHON) setup.py install \
-		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" --force
-	$(INSTALL_PYTHON_PROG) src/pygrub $(DESTDIR)/$(BINDIR)/pygrub
+		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
+		--install-scripts=$(DESTDIR)/$(PRIVATE_BINDIR) --force
+	$(INSTALL_PYTHON_PROG) src/pygrub $(DESTDIR)/$(PRIVATE_BINDIR)/pygrub
 	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
+	ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR)
 
 .PHONY: clean
 clean:

George Dunlap

2012-May-09 10:51 UTC

head link

[PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 52f45b258ccc -r 794778a6e9fa tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Wed May 09 11:39:43 2012 +0100
+++ b/tools/libxl/libxl_bootloader.c	Wed May 09 11:42:19 2012 +0100
@@ -15,6 +15,7 @@
 #include "libxl_osdeps.h" /* must come before any other headers */
 
 #include <termios.h>
+#include <string.h>
 
 #include "libxl_internal.h"
 
@@ -398,6 +399,11 @@ int libxl_run_bootloader(libxl_ctx *ctx,
         goto out_close;
     }
 
+    if ( !strncmp(info->u.pv.bootloader, "/usr/bin/pygrub", 20) )
+        LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                   "WARNING:
bootloader=''/usr/bin/pygrub'' "             \
+                   "is deprecated; use
bootloader=''pygrub'' instead");
+    
     bootloader = libxl__abs_path(gc, info->u.pv.bootloader,
                                  libxl__libexec_path());
     if ( bootloader == NULL ) {

Ian Campbell

2012-May-09 13:39 UTC

head link

Re: [PATCH 1 of 3 RESEND] libxl: Look for bootloader in libexec path

On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:> If the full path for a bootloader (such as pygrub or xenpvnetboot) is not
> given, look for it in the libexec path.
xend used to do something similar, see tools/python/xen/util/auxbin.py
where it searches in:
        SEARCHDIRS = [ BINDIR, SBINDIR, LIBEXEC, PRIVATE_BINDIR, XENFIRMWAREDIR
]
where:
        SBINDIR="/usr/sbin"
        BINDIR="/usr/bin"
        LIBEXEC="/usr/lib/xen/bin"
        PRIVATE_BINDIR="/usr/lib/xen/bin"
        XENFIRMWAREDIR="/usr/lib/xen/boot"

I think since libxl uses execvp the first two are handled by path
expansion, the next two are what you are adding (so has priority over
$PATH in your patch, which I think is OK) and the last one is irrelevant
in this context (xend uses the same function for other contexts).

However in order to not differ on the first two execvp needs to be given
the opportunity to search the path, which means that you need to check
of your newly constructed bootloader exists, and if it doesn''t then try
and continue with the relative path not the absolute one.

Also since this patch comes before the rename+symlink patch as it stands
this patch will temporarily break people who just have "pygrub"
without
a path in their config.

Ian.
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r 8a86d841e6d4 -r e43157a86933 tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c	Tue May 08 13:36:24 2012 +0200
> +++ b/tools/libxl/libxl_bootloader.c	Wed May 09 11:38:50 2012 +0100
> @@ -333,6 +333,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>  
>      char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
>      char *tempdir;
> +    const char *bootloader = NULL;
>  
>      char *dom_console_xs_path;
>      char dom_console_slave_tty_path[PATH_MAX];
> @@ -397,6 +398,13 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>          goto out_close;
>      }
>  
> +    bootloader = libxl__abs_path(gc, info->u.pv.bootloader,
> +                                 libxl__libexec_path());
> +    if ( bootloader == NULL ) {
> +        rc = ERROR_NOMEM;
> +        goto out_close;
> +    }
> +
>      /*
>       * We need to present the bootloader''s tty as a pty slave
that xenconsole
>       * can access.  Since the bootloader itself needs a pty slave,
> @@ -417,7 +425,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>      dom_console_xs_path = libxl__sprintf(gc, "%s/console/tty",
libxl__xs_get_dompath(gc, domid));
>      libxl__xs_write(gc, XBT_NULL, dom_console_xs_path, "%s",
dom_console_slave_tty_path);
>  
> -    pid = fork_exec_bootloader(&bootloader_fd,
info->u.pv.bootloader, args);
> +    pid = fork_exec_bootloader(&bootloader_fd, bootloader, args);
>      if (pid < 0) {
>          goto out_close;
>      }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Ian Campbell

2012-May-09 13:41 UTC

head link

Re: [PATCH 2 of 3 RESEND] tools: Install pv bootloaders in libexec rather than /usr/bin

On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:> pygrub and xenpvnetboot are meant to be run by tools, and not by the user,
> and thus should be in /usr/lib/xen/bin rather than /usr/bin.
> 
> Because most config files will still have an absolute path pointing to
> /usr/bin/pygrub, make a symbolic link that we will deprecate.
You forgot your S-o-b.

I think that moving xenpvnetboot out of $PATH before 4.2 is a good idea,
so we don''t have to worry about compatibility concerns since we never
previously released with xenpvnetboot.

I think this patch should go in before 1 of 3 to minimise the window of
breakage (see my comment on that patch).

Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff -r e43157a86933 -r 52f45b258ccc tools/misc/Makefile
> --- a/tools/misc/Makefile	Wed May 09 11:38:50 2012 +0100
> +++ b/tools/misc/Makefile	Wed May 09 11:39:43 2012 +0100
> @@ -18,7 +18,7 @@ SUBDIRS-$(CONFIG_LOMOUNT) += lomount
>  SUBDIRS-$(CONFIG_MINITERM) += miniterm
>  SUBDIRS := $(SUBDIRS-y)
>  
> -INSTALL_BIN-y := xencons xenpvnetboot
> +INSTALL_BIN-y := xencons
>  INSTALL_BIN-$(CONFIG_X86) += xen-detect
>  INSTALL_BIN := $(INSTALL_BIN-y)
>  
> @@ -27,6 +27,9 @@ INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx
>  INSTALL_SBIN-$(CONFIG_MIGRATE) += xen-hptool
>  INSTALL_SBIN := $(INSTALL_SBIN-y)
>  
> +INSTALL_PRIVBIN-y := xenpvnetboot
> +INSTALL_PRIVBIN := $(INSTALL_PRIVBIN-y)
> +
>  # Include configure output (config.h) to headers search path
>  CFLAGS += -I$(XEN_ROOT)/tools
>  
> @@ -41,8 +44,10 @@ build: $(TARGETS)
>  install: build
>  	$(INSTALL_DIR) $(DESTDIR)$(BINDIR)
>  	$(INSTALL_DIR) $(DESTDIR)$(SBINDIR)
> +	$(INSTALL_DIR) $(DESTDIR)$(PRIVATE_BINDIR)
>  	$(INSTALL_PYTHON_PROG) $(INSTALL_BIN) $(DESTDIR)$(BINDIR)
>  	$(INSTALL_PYTHON_PROG) $(INSTALL_SBIN) $(DESTDIR)$(SBINDIR)
> +	$(INSTALL_PYTHON_PROG) $(INSTALL_PRIVBIN) $(DESTDIR)$(PRIVATE_BINDIR)
>  	set -e; for d in $(SUBDIRS); do $(MAKE) -C $$d install-recurse; done
>  
>  .PHONY: clean
> diff -r e43157a86933 -r 52f45b258ccc tools/pygrub/Makefile
> --- a/tools/pygrub/Makefile	Wed May 09 11:38:50 2012 +0100
> +++ b/tools/pygrub/Makefile	Wed May 09 11:39:43 2012 +0100
> @@ -11,9 +11,11 @@ build:
>  .PHONY: install
>  install: all
>  	CC="$(CC)" CFLAGS="$(CFLAGS)" $(PYTHON) setup.py
install \
> -		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" --force
> -	$(INSTALL_PYTHON_PROG) src/pygrub $(DESTDIR)/$(BINDIR)/pygrub
> +		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
> +		--install-scripts=$(DESTDIR)/$(PRIVATE_BINDIR) --force
> +	$(INSTALL_PYTHON_PROG) src/pygrub $(DESTDIR)/$(PRIVATE_BINDIR)/pygrub
>  	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
> +	ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR)
>  
>  .PHONY: clean
>  clean:
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Ian Campbell

2012-May-09 13:43 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r 52f45b258ccc -r 794778a6e9fa tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c	Wed May 09 11:39:43 2012 +0100
> +++ b/tools/libxl/libxl_bootloader.c	Wed May 09 11:42:19 2012 +0100
> @@ -15,6 +15,7 @@
>  #include "libxl_osdeps.h" /* must come before any other headers
*/
>  
>  #include <termios.h>
> +#include <string.h>
>  
>  #include "libxl_internal.h"
>  
> @@ -398,6 +399,11 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>          goto out_close;
>      }
>  
> +    if ( !strncmp(info->u.pv.bootloader, "/usr/bin/pygrub",
20) )
Why strncmp and not just strcmp? And why 20? AFAIK
strlen("/usr/bin/pygrub") == 15 or 16 or so...

Ian.
> +        LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
> +                   "WARNING:
bootloader=''/usr/bin/pygrub'' "             \
> +                   "is deprecated; use
bootloader=''pygrub'' instead");
> +    
>      bootloader = libxl__abs_path(gc, info->u.pv.bootloader,
>                                   libxl__libexec_path());
>      if ( bootloader == NULL ) {
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

George Dunlap

2012-May-09 14:01 UTC

head link

Re: [PATCH 1 of 3 RESEND] libxl: Look for bootloader in libexec path

On 09/05/12 14:39, Ian Campbell wrote:> I think since libxl uses execvp the first two are handled by path
> expansion, the next two are what you are adding (so has priority over
> $PATH in your patch, which I think is OK) and the last one is irrelevant
> in this context (xend uses the same function for other contexts).
>
> However in order to not differ on the first two execvp needs to be given
> the opportunity to search the path, which means that you need to check
> of your newly constructed bootloader exists, and if it doesn''t
then try
> and continue with the relative path not the absolute one.
>
> Also since this patch comes before the rename+symlink patch as it stands
> this patch will temporarily break people who just have "pygrub"
without
> a path in their config.Ah, I didn''t realize that xl would already search the path (via
execvp).

I think it should be pretty straightforward to add a check for 
existence, and if not fall back to letting execvp search the path.

If I do that, arguably this patch would still come first in the series?  
Not that it matters much, really.

  -George
>
> Ian.
>
>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>
>> diff -r 8a86d841e6d4 -r e43157a86933 tools/libxl/libxl_bootloader.c
>> --- a/tools/libxl/libxl_bootloader.c	Tue May 08 13:36:24 2012 +0200
>> +++ b/tools/libxl/libxl_bootloader.c	Wed May 09 11:38:50 2012 +0100
>> @@ -333,6 +333,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>>
>>       char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
>>       char *tempdir;
>> +    const char *bootloader = NULL;
>>
>>       char *dom_console_xs_path;
>>       char dom_console_slave_tty_path[PATH_MAX];
>> @@ -397,6 +398,13 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>>           goto out_close;
>>       }
>>
>> +    bootloader = libxl__abs_path(gc, info->u.pv.bootloader,
>> +                                 libxl__libexec_path());
>> +    if ( bootloader == NULL ) {
>> +        rc = ERROR_NOMEM;
>> +        goto out_close;
>> +    }
>> +
>>       /*
>>        * We need to present the bootloader''s tty as a pty
slave that xenconsole
>>        * can access.  Since the bootloader itself needs a pty slave,
>> @@ -417,7 +425,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>>       dom_console_xs_path = libxl__sprintf(gc,
"%s/console/tty", libxl__xs_get_dompath(gc, domid));
>>       libxl__xs_write(gc, XBT_NULL, dom_console_xs_path,
"%s", dom_console_slave_tty_path);
>>
>> -    pid = fork_exec_bootloader(&bootloader_fd,
info->u.pv.bootloader, args);
>> +    pid = fork_exec_bootloader(&bootloader_fd, bootloader, args);
>>       if (pid<  0) {
>>           goto out_close;
>>       }
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

George Dunlap

2012-May-09 14:48 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

On 09/05/12 14:43, Ian Campbell wrote:> On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:
>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>
>> diff -r 52f45b258ccc -r 794778a6e9fa tools/libxl/libxl_bootloader.c
>> --- a/tools/libxl/libxl_bootloader.c	Wed May 09 11:39:43 2012 +0100
>> +++ b/tools/libxl/libxl_bootloader.c	Wed May 09 11:42:19 2012 +0100
>> @@ -15,6 +15,7 @@
>>   #include "libxl_osdeps.h" /* must come before any other
headers */
>>
>>   #include<termios.h>
>> +#include<string.h>
>>
>>   #include "libxl_internal.h"
>>
>> @@ -398,6 +399,11 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>>           goto out_close;
>>       }
>>
>> +    if ( !strncmp(info->u.pv.bootloader,
"/usr/bin/pygrub", 20) )
> Why strncmp and not just strcmp? And why 20? AFAIK
> strlen("/usr/bin/pygrub") == 15 or 16 or so...ISTR in the past build processes throwing warnings that strcmp() is 
unsafe, and since warnings turn to errors, pre-emptively used the
"safe"
version instead.  Let me give it a try; it should be perfectly safe for 
us, and will remove the issue with manually syncronizing string with its 
size.

  -George

Ian Jackson

2012-May-10 11:36 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

George Dunlap writes ("Re: [Xen-devel] [PATCH 3 of 3 RESEND] libxl: Warn
that /usr/bin/pygrub is deprecated"):> On 09/05/12 14:43, Ian Campbell wrote:
> > On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:
> >> +    if ( !strncmp(info->u.pv.bootloader,
"/usr/bin/pygrub", 20) )
> > Why strncmp and not just strcmp? And why 20? AFAIK
> > strlen("/usr/bin/pygrub") == 15 or 16 or so...
>
> ISTR in the past build processes throwing warnings that strcmp() is 
> unsafe, and since warnings turn to errors, pre-emptively used the
"safe"
> version instead.
Boggle.  Any such build processes need to be taken out and shot.
There is nothing wrong with strcmp.  Are you sure you''re not thinking
of strcat or sprintf ?
>  Let me give it a try; it should be perfectly safe for 
> us, and will remove the issue with manually syncronizing string with its 
> size.
Right, yes.

Ian.

George Dunlap

2012-May-10 11:37 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

On 10/05/12 12:36, Ian Jackson wrote:> George Dunlap writes ("Re: [Xen-devel] [PATCH 3 of 3 RESEND] libxl:
Warn that /usr/bin/pygrub is deprecated"):
>> On 09/05/12 14:43, Ian Campbell wrote:
>>> On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:
>>>> +    if ( !strncmp(info->u.pv.bootloader,
"/usr/bin/pygrub", 20) )
>>> Why strncmp and not just strcmp? And why 20? AFAIK
>>> strlen("/usr/bin/pygrub") == 15 or 16 or so...
>> ISTR in the past build processes throwing warnings that strcmp() is
>> unsafe, and since warnings turn to errors, pre-emptively used the
"safe"
>> version instead.
> Boggle.  Any such build processes need to be taken out and shot.
> There is nothing wrong with strcmp.  Are you sure you''re not
thinking
> of strcat or sprintf ?Perhaps, or strcpy.  At any rate, it compiles just fine with strcmp(), 
so I''ll include that in the next post.

  -George

Tim Deegan

2012-May-10 11:44 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

At 12:36 +0100 on 10 May (1336653395), Ian Jackson
wrote:> George Dunlap writes ("Re: [Xen-devel] [PATCH 3 of 3 RESEND] libxl:
Warn that /usr/bin/pygrub is deprecated"):
> > On 09/05/12 14:43, Ian Campbell wrote:
> > > On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:
> > >> +    if ( !strncmp(info->u.pv.bootloader,
"/usr/bin/pygrub", 20) )
> > > Why strncmp and not just strcmp? And why 20? AFAIK
> > > strlen("/usr/bin/pygrub") == 15 or 16 or so...
> >
> > ISTR in the past build processes throwing warnings that strcmp() is 
> > unsafe, and since warnings turn to errors, pre-emptively used the
"safe"
> > version instead.
> 
> Boggle.  Any such build processes need to be taken out and shot.
> There is nothing wrong with strcmp.  Are you sure you''re not
thinking
> of strcat or sprintf ?
If the user controlled both the length and contents of
info->u.pv.bootloader, it could cause this to overrun that buffer and
cause a SEGV.  So, sadly, strcmp goes on the ''just never use
it'' list
for many people.

Tim.

George Dunlap

2012-May-10 11:47 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

On 10/05/12 12:44, Tim Deegan wrote:> At 12:36 +0100 on 10 May (1336653395), Ian Jackson wrote:
>> George Dunlap writes ("Re: [Xen-devel] [PATCH 3 of 3 RESEND]
libxl: Warn that /usr/bin/pygrub is deprecated"):
>>> On 09/05/12 14:43, Ian Campbell wrote:
>>>> On Wed, 2012-05-09 at 11:51 +0100, George Dunlap wrote:
>>>>> +    if ( !strncmp(info->u.pv.bootloader,
"/usr/bin/pygrub", 20) )
>>>> Why strncmp and not just strcmp? And why 20? AFAIK
>>>> strlen("/usr/bin/pygrub") == 15 or 16 or so...
>>> ISTR in the past build processes throwing warnings that strcmp() is
>>> unsafe, and since warnings turn to errors, pre-emptively used the
"safe"
>>> version instead.
>> Boggle.  Any such build processes need to be taken out and shot.
>> There is nothing wrong with strcmp.  Are you sure you''re not
thinking
>> of strcat or sprintf ?
> If the user controlled both the length and contents of
> info->u.pv.bootloader, it could cause this to overrun that buffer and
> cause a SEGV.  So, sadly, strcmp goes on the ''just never use
it'' list
> for many people.Hmm, yes, I suppose it''s *technically* possible that even when
comparing
to a static string, if info->u.pv.bootloader contains a short, 
non-null-terminated string, and were close to the edge of a page, it 
could cause a SEGV.  But using strncmp wouldn''t solve that, would it?

  -George

Tim Deegan

2012-May-10 12:10 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

At 12:47 +0100 on 10 May (1336654044), George Dunlap
wrote:> On 10/05/12 12:44, Tim Deegan wrote:
> >If the user controlled both the length and contents of
> >info->u.pv.bootloader, it could cause this to overrun that buffer
and
> >cause a SEGV.  So, sadly, strcmp goes on the ''just never use
it'' list
> >for many people.
> 
> Hmm, yes, I suppose it''s *technically* possible that even when
comparing
> to a static string, if info->u.pv.bootloader contains a short, 
> non-null-terminated string, and were close to the edge of a page, it 
> could cause a SEGV.  But using strncmp wouldn''t solve that, would
it?
Yes - you give it the length of the info->u.pv.bootloader buffer and it
guards against from exactly this problem.  That''s assuming you
allocated
it yourself and filled it with user-supplied bytes.  If the user
supplied the buffer, of course, you''re forced to trust them and
strncmp() doesn''t buy you anything.

Tim.

Ian Jackson

2012-May-10 13:15 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Tim Deegan writes ("Re: [Xen-devel] [PATCH 3 of 3 RESEND] libxl: Warn that
/usr/bin/pygrub is deprecated"):> At 12:36 +0100 on 10 May (1336653395), Ian Jackson wrote:
> > Boggle.  Any such build processes need to be taken out and shot.
> > There is nothing wrong with strcmp.  Are you sure you''re not
thinking
> > of strcat or sprintf ?
> 
> If the user controlled both the length and contents of
> info->u.pv.bootloader, it could cause this to overrun that buffer and
> cause a SEGV.  So, sadly, strcmp goes on the ''just never use
it'' list
> for many people.
info->u.pv.bootloader is a string.  The in-process caller of libxl
is required to provide a nul-terminated buffer.  In general, strcmp is
correct for user-provided strings when the string is a string.

I think perhaps people have been confused by the habit of some kernel
ABI designers to write something like:
   struct mumble {
       char mumblename[16];
       ...
   };
and then to allow callers to supply 16-octet mumblenames (necessarily,
then, without a trailing nul), or shorter mumblenames (with trailing
nul).  In that case strncmp is indeed necessary.

But these kind of interfaces are a rarity in userland and certainly
libxl''s API/ABI doesn''t have anything like that.  In the case
of
info->u.pv.bootloader the string is from malloc and the "buffer
length" isn''t even recorded anywhere so there would be no correct
value to pass to strncmp that wasn''t ~(size_t)0.

Ian.

Tim Deegan

2012-May-10 13:20 UTC

head link

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

At 14:15 +0100 on 10 May (1336659339), Ian Jackson
wrote:> Tim Deegan writes ("Re: [Xen-devel] [PATCH 3 of 3 RESEND] libxl: Warn
that /usr/bin/pygrub is deprecated"):
> > At 12:36 +0100 on 10 May (1336653395), Ian Jackson wrote:
> > > Boggle.  Any such build processes need to be taken out and shot.
> > > There is nothing wrong with strcmp.  Are you sure you''re
not thinking
> > > of strcat or sprintf ?
> > 
> > If the user controlled both the length and contents of
> > info->u.pv.bootloader, it could cause this to overrun that buffer
and
> > cause a SEGV.  So, sadly, strcmp goes on the ''just never use
it'' list
> > for many people.
> 
> info->u.pv.bootloader is a string.  The in-process caller of libxl
> is required to provide a nul-terminated buffer.  In general, strcmp is
> correct for user-provided strings when the string is a string.
Sure, in this case, strcmp is fine; I was talking about the reasons why
people are scared of it.

Tim.

Xen devel - May 2012 - [PATCH 0 of 3 RESEND] tools: Move bootloaders to libexec directory

[PATCH 0 of 3 RESEND] tools: Move bootloaders to libexec directory

[PATCH 1 of 3 RESEND] libxl: Look for bootloader in libexec path

[PATCH 2 of 3 RESEND] tools: Install pv bootloaders in libexec rather than /usr/bin

[PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 1 of 3 RESEND] libxl: Look for bootloader in libexec path

Re: [PATCH 2 of 3 RESEND] tools: Install pv bootloaders in libexec rather than /usr/bin

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 1 of 3 RESEND] libxl: Look for bootloader in libexec path

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated

Re: [PATCH 3 of 3 RESEND] libxl: Warn that /usr/bin/pygrub is deprecated