| Message ID | 1461682343-20597-11-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 26, 2016 at 04:52:19PM +0200, Roger Pau Monne wrote: [...] > @@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s); > _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid); > /* Return the system-wide default device model */ > _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc); > -_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid, > - uint32_t domid, > - const char *format, ...) PRINTF_ATTRIBUTE(4, 5); Why does this not work with clang? Wei.
On 26/04/16 16:29, Wei Liu wrote: > On Tue, Apr 26, 2016 at 04:52:19PM +0200, Roger Pau Monne wrote: > [...] >> @@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s); >> _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid); >> /* Return the system-wide default device model */ >> _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc); >> -_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid, >> - uint32_t domid, >> - const char *format, ...) PRINTF_ATTRIBUTE(4, 5); > Why does this not work with clang? It is a security consideration. Passing anything other than a string literal to a printf-style function is opening a can of worms if an untrusted entity can influence the content of the string. I guess clang is better at spotting parameters passed like this than GCC. ~Andrew
On Tue, Apr 26, 2016 at 04:30:36PM +0100, Andrew Cooper wrote: > On 26/04/16 16:29, Wei Liu wrote: > > On Tue, Apr 26, 2016 at 04:52:19PM +0200, Roger Pau Monne wrote: > > [...] > >> @@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s); > >> _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid); > >> /* Return the system-wide default device model */ > >> _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc); > >> -_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid, > >> - uint32_t domid, > >> - const char *format, ...) PRINTF_ATTRIBUTE(4, 5); > > Why does this not work with clang? > > It is a security consideration. > > Passing anything other than a string literal to a printf-style function > is opening a can of worms if an untrusted entity can influence the > content of the string. > I see. I didn't look closely into the function body. > I guess clang is better at spotting parameters passed like this than GCC. > Sigh. I can't say I like turning that into a macro though. On the other hand there doesn't seem to be an elegant way of solving that. Roger, please at least make it look like a macro. Say, name it DEVICE_MODEL_XS_PATH or something. Wei. > ~Andrew
Wei Liu writes ("Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions"):
> Sigh. I can't say I like turning that into a macro though. On the other
> hand there doesn't seem to be an elegant way of solving that.
Well, other than suppressing the warning somehow.
> Roger, please at least make it look like a macro. Say, name it
> DEVICE_MODEL_XS_PATH or something.
I'm not sure I agree. I think if we can make it a function-like
macro, then it is fine to give it a function-like name. (Note that
many libc functions might be macros; getc is traditionally a #define.)
Obviously it can't be function-like in that you must always pass it a
literal, but violations of that constraint will be detected by the
compiler.
Ian.
On Thu, Apr 28, 2016 at 06:26:09PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions"): > > Sigh. I can't say I like turning that into a macro though. On the other > > hand there doesn't seem to be an elegant way of solving that. > > Well, other than suppressing the warning somehow. > I don't think we want to suppress the warning. It's useful. > > Roger, please at least make it look like a macro. Say, name it > > DEVICE_MODEL_XS_PATH or something. > > I'm not sure I agree. I think if we can make it a function-like > macro, then it is fine to give it a function-like name. (Note that > many libc functions might be macros; getc is traditionally a #define.) > > Obviously it can't be function-like in that you must always pass it a > literal, but violations of that constraint will be detected by the > compiler. I've pushed the series from Roger to staging because it fixes clang compilation. I don't have strong opinion on what the name of macro should look like so feel free to send a patch to change that. Wei. > > Ian.
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c index e7b765b..3b30f8a 100644 --- a/tools/libxl/libxl_internal.c +++ b/tools/libxl/libxl_internal.c @@ -554,22 +554,6 @@ void libxl__update_domain_configuration(libxl__gc *gc, dst->b_info.video_memkb = src->b_info.video_memkb; } -char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid, - uint32_t domid, const char *format, ...) -{ - char *s, *fmt; - va_list ap; - - fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid, - domid, format); - - va_start(ap, format); - s = libxl__vsprintf(gc, fmt, ap); - va_end(ap); - - return s; -} - /* * Local variables: * mode: C diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 8e2cf3e..0879e4c 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -636,7 +636,7 @@ _hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t new_size) NN1; /* print @fmt into an allocated string large enoughto contain the result. * (similar to gc'd asprintf(3)). */ _hidden char *libxl__sprintf(libxl__gc *gc_opt, const char *fmt, ...) PRINTF_ATTRIBUTE(2, 3) NN1; -_hidden char *libxl__vsprintf(libxl__gc *gc, const char *format, va_list ap); +_hidden char *libxl__vsprintf(libxl__gc *gc, const char *format, va_list ap) PRINTF_ATTRIBUTE(2, 0); /* duplicate the string @c (similar to a gc'd strdup(3)). */ _hidden char *libxl__strdup(libxl__gc *gc_opt, const char *c /* may be NULL */) NN1; @@ -709,7 +709,7 @@ _hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid); */ int libxl__xs_vprintf(libxl__gc *gc, xs_transaction_t t, - const char *path, const char *fmt, va_list ap); + const char *path, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(4, 0); int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t, const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 5); @@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s); _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid); /* Return the system-wide default device model */ _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc); -_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid, - uint32_t domid, - const char *format, ...) PRINTF_ATTRIBUTE(4, 5); + +#define libxl__device_model_xs_path(gc, dm_domid, domid, fmt, _a...) \ + libxl__sprintf(gc, "/local/domain/%u/device-model/%u" fmt, dm_domid, \ + domid, ##_a) /* * Calling context and GC for event-generating functions: diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c index 5fe642a..d3def6b 100644 --- a/tools/libxl/libxl_save_helper.c +++ b/tools/libxl/libxl_save_helper.c @@ -52,6 +52,7 @@ /*----- logger -----*/ +__attribute__((format(printf, 5, 0))) static void tellparent_vmessage(xentoollog_logger *logger_in, xentoollog_level level, int errnoval,
Or else clang complains with: error: format string is not a string literal This looks quite pedantic, but I don't know of any other way to get rid of those errors. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- tools/libxl/libxl_internal.c | 16 ---------------- tools/libxl/libxl_internal.h | 11 ++++++----- tools/libxl/libxl_save_helper.c | 1 + 3 files changed, 7 insertions(+), 21 deletions(-)