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

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

[George Dunlap]

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

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

[George Dunlap]

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;
     }

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

[Ian Campbell]

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

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

[George Dunlap]

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
>

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

[George Dunlap]

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:

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

[Ian Campbell]

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

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

[George Dunlap]

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 ) {

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

[Ian Campbell]

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

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

[George Dunlap]

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

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

[Ian Jackson]

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.

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

[George Dunlap]

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

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

[Tim Deegan]

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.

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

[George Dunlap]

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

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

[Tim Deegan]

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.

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

[Ian Jackson]

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.

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

[Tim Deegan]

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.