From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shriram Rajagopalan Subject: Re: Re: c/s 23253:a3db6b91f32d causes build failure with gcc 4.4.3-4ubuntu5 Date: Fri, 6 May 2011 09:38:12 -0500 Message-ID: References: <1304685218.26692.154.camel@zakaz.uk.xensource.com> <1304686694.26692.161.camel@zakaz.uk.xensource.com> Reply-To: rshriram@cs.ubc.ca Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0048419903==" Return-path: In-Reply-To: <1304686694.26692.161.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: George Dunlap , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org --===============0048419903== Content-Type: multipart/alternative; boundary=00151750e552d7feb804a29c72ba --00151750e552d7feb804a29c72ba Content-Type: text/plain; charset=ISO-8859-1 On Fri, May 6, 2011 at 7:58 AM, Ian Campbell wrote: > On Fri, 2011-05-06 at 13:50 +0100, George Dunlap wrote: > > Unfortunately not. > > > > Looks like we could either make libxl__domain_bios() a #define (not > > tested), or make the argument "%s" (which seems to work, patch > > attached). > > > > Adding a format string of just a string seems kind of dumb, but I > > don't think this is a really hot path... > > It's actually sensible to use "%s" when the string might be untrusted > but in this case it's really just a string literal, I guess the > laundering through a function is enough to hide that from gcc. Using %s > is the best fix, I think. > > Is this going to go in as a patch? I havent seen any so far (or probably missed it). shriram > Ian. > > > > > -George > > > > > > On Fri, May 6, 2011 at 1:33 PM, Ian Campbell > wrote: > > > On Fri, 2011-05-06 at 12:14 +0100, George Dunlap wrote: > > > > > >> The line in question is: > > >> libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/bios", path), > > >> libxl__domain_bios(gc, info)); > > >> > > >> Looks like libxl__xs_write() is expecting the 4th argument to be a > > >> format string...? > > > > > > Yes, and hence it needs to be a const char * not a char *, so we should > > > change both libxl__domain_bios and libxl__xs_write I think. > > > > > > Does this help? It works for me, but my compiler doesn't appear to > > > complain in this way... > > > > > > Ian. > > > > > > 8<------------------------------------------- > > > > > > # HG changeset patch > > > # User Ian Campbell > > > # Date 1304685175 -3600 > > > # Node ID 4e5487962178e7affd7d7d0341a90dde8c60915e > > > # Parent faca1c90188e536eb0f02992c766d06759be376f > > > libxl: libxl__xs_write format string should be const. > > > > > > George Dunlap reports that gcc 4.4.3 complains: > > > libxl_dm.c: In function libxl__create_device_mode: > > > libxl_dm.c:776: error: format not a string literal and no format > arguments > > > And indeed the format argument here is a char * from > libxl__domain_bios(). > > > > > > Make the argument to libxl__xs_write a const char * and change > > > libxl__domain_bios to return a const char too. > > > > > > Signed-off-by: Ian Campbell > > > > > > diff -r faca1c90188e -r 4e5487962178 tools/libxl/libxl_dm.c > > > --- a/tools/libxl/libxl_dm.c Fri May 06 13:14:24 2011 +0100 > > > +++ b/tools/libxl/libxl_dm.c Fri May 06 13:32:55 2011 +0100 > > > @@ -68,12 +68,12 @@ const char *libxl__domain_device_model(l > > > return dm; > > > } > > > > > > -static char *libxl__domain_bios(libxl__gc *gc, > > > +static const char *libxl__domain_bios(libxl__gc *gc, > > > libxl_device_model_info *info) > > > { > > > switch (info->device_model_version) { > > > - case 1: return libxl__strdup(gc, "rombios"); > > > - case 2: return libxl__strdup(gc, "seabios"); > > > + case 1: return "rombios"; > > > + case 2: return "seabios"; > > > default:return NULL; > > > } > > > } > > > diff -r faca1c90188e -r 4e5487962178 tools/libxl/libxl_internal.h > > > --- a/tools/libxl/libxl_internal.h Fri May 06 13:14:24 2011 +0100 > > > +++ b/tools/libxl/libxl_internal.h Fri May 06 13:32:55 2011 +0100 > > > @@ -153,7 +153,7 @@ _hidden char **libxl__xs_kvs_of_flexarra > > > _hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t, > > > char *dir, char **kvs); > > > _hidden int libxl__xs_write(libxl__gc *gc, xs_transaction_t t, > > > - char *path, char *fmt, ...) PRINTF_ATTRIBUTE(4, 5); > > > + char *path, const char *fmt, ...) > PRINTF_ATTRIBUTE(4, 5); > > > /* Each fn returns 0 on success. > > > * On error: returns -1, sets errno (no logging) */ > > > > > > diff -r faca1c90188e -r 4e5487962178 tools/libxl/libxl_xshelp.c > > > --- a/tools/libxl/libxl_xshelp.c Fri May 06 13:14:24 2011 +0100 > > > +++ b/tools/libxl/libxl_xshelp.c Fri May 06 13:32:55 2011 +0100 > > > @@ -69,7 +69,7 @@ int libxl__xs_writev(libxl__gc *gc, xs_t > > > } > > > > > > int libxl__xs_write(libxl__gc *gc, xs_transaction_t t, > > > - char *path, char *fmt, ...) > > > + char *path, const char *fmt, ...) > > > { > > > libxl_ctx *ctx = libxl__gc_owner(gc); > > > char *s; > > > > > > > > > > > > _______________________________________________ > > > Xen-devel mailing list > > > Xen-devel@lists.xensource.com > > > http://lists.xensource.com/xen-devel > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > --00151750e552d7feb804a29c72ba Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Fri, May 6, 2011 at 7:58 AM, Ian Campbell <Ian.Campbe= ll@eu.citrix.com> wrote:
On Fri, 2011-05-06 at 13:50 +0100, George Dunlap wrote: > Unfortunately not.
>
> Looks like we could either make libxl__domain_bios() a #define (not > tested), or make the argument "%s" (which seems to work, pat= ch
> attached).
>
> Adding a format string of just a string seems kind of dumb, but I
> don't think this is a really hot path...

It's actually sensible to use "%s" when the string migh= t be untrusted
but in this case it's really just a string literal, I guess the
laundering through a function is enough to hide that from gcc. Using %s
is the best fix, I think.

Is this going to go in= as a patch? I havent seen any so far (or probably missed it).

shrir= am
Ian.

>
> =A0-George
>
>
> On Fri, May 6, 2011 at 1:33 PM, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> > On Fri, 2011-05-06 at 12:14 +0100, George Dunlap wrote:
> >
> >> The line in question is:
> >> =A0 =A0 libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, &quo= t;%s/bios", path),
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 libxl__domain_bios(gc= , info));
> >>
> >> Looks like libxl__xs_write() is expecting the 4th argument to= be a
> >> format string...?
> >
> > Yes, and hence it needs to be a const char * not a char *, so we = should
> > change both libxl__domain_bios and libxl__xs_write I think.
> >
> > Does this help? It works for me, but my compiler doesn't appe= ar to
> > complain in this way...
> >
> > Ian.
> >
> > 8<-------------------------------------------
> >
> > # HG changeset patch
> > # User Ian Campbell <ian.campbell@citrix.com>
> > # Date 1304685175 -3600
> > # Node ID 4e5487962178e7affd7d7d0341a90dde8c60915e
> > # Parent =A0faca1c90188e536eb0f02992c766d06759be376f
> > libxl: libxl__xs_write format string should be const.
> >
> > George Dunlap reports that gcc 4.4.3 complains:
> > =A0 =A0 =A0 =A0libxl_dm.c: In function libxl__create_device_mode:=
> > =A0 =A0 =A0 =A0libxl_dm.c:776: error: format not a string literal= and no format arguments
> > And indeed the format argument here is a char * from libxl__domai= n_bios().
> >
> > Make the argument to libxl__xs_write a const char * and change > > libxl__domain_bios to return a const char too.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > diff -r faca1c90188e -r 4e5487962178 tools/libxl/libxl_dm.c
> > --- a/tools/libxl/libxl_dm.c =A0 =A0Fri May 06 13:14:24 2011 +010= 0
> > +++ b/tools/libxl/libxl_dm.c =A0 =A0Fri May 06 13:32:55 2011 +010= 0
> > @@ -68,12 +68,12 @@ const char *libxl__domain_device_model(l
> > =A0 =A0 return dm;
> > =A0}
> >
> > -static char *libxl__domain_bios(libxl__gc *gc,
> > +static const char *libxl__domain_bios(libxl__gc *gc,
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 l= ibxl_device_model_info *info)
> > =A0{
> > =A0 =A0 switch (info->device_model_version) {
> > - =A0 =A0case 1: return libxl__strdup(gc, "rombios"); > > - =A0 =A0case 2: return libxl__strdup(gc, "seabios"); > > + =A0 =A0case 1: return "rombios";
> > + =A0 =A0case 2: return "seabios";
> > =A0 =A0 default:return NULL;
> > =A0 =A0 }
> > =A0}
> > diff -r faca1c90188e -r 4e5487962178 tools/libxl/libxl_internal.h=
> > --- a/tools/libxl/libxl_internal.h =A0 =A0 =A0Fri May 06 13:14:24= 2011 +0100
> > +++ b/tools/libxl/libxl_internal.h =A0 =A0 =A0Fri May 06 13:32:55= 2011 +0100
> > @@ -153,7 +153,7 @@ _hidden char **libxl__xs_kvs_of_flexarra
> > =A0_hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t= ,
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 char *dir, char **kvs); > > =A0_hidden int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,=
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 char *path, char *fmt, ...)= PRINTF_ATTRIBUTE(4, 5);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 char *path, const char *fmt= , ...) PRINTF_ATTRIBUTE(4, 5);
> > =A0 =A0/* Each fn returns 0 on success.
> > =A0 =A0 * On error: returns -1, sets errno (no logging) */
> >
> > diff -r faca1c90188e -r 4e5487962178 tools/libxl/libxl_xshelp.c > > --- a/tools/libxl/libxl_xshelp.c =A0 =A0 =A0 =A0Fri May 06 13:14:= 24 2011 +0100
> > +++ b/tools/libxl/libxl_xshelp.c =A0 =A0 =A0 =A0Fri May 06 13:32:= 55 2011 +0100
> > @@ -69,7 +69,7 @@ int libxl__xs_writev(libxl__gc *gc, xs_t
> > =A0}
> >
> > =A0int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 char *path, char *fmt, ...)=
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 char *path, const char *fmt= , ...)
> > =A0{
> > =A0 =A0 libxl_ctx *ctx =3D libxl__gc_owner(gc);
> > =A0 =A0 char *s;
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.= xensource.com
> > http://lists.xensource.com/xen-devel
> >



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.= com
http://l= ists.xensource.com/xen-devel

--00151750e552d7feb804a29c72ba-- --===============0048419903== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============0048419903==--