Ian Jackson
2012-Jul-27 16:21 UTC
[PATCH v2] libxl: enforce prohibitions of internal callers
libxl_internal.h says:
* Functions using LIBXL__INIT_EGC may *not* generally be called from
* within libxl, because libxl__egc_cleanup may call back into the
* application. ...
and
* ... [Functions which take an ao_how] MAY NOT
* be called from inside libxl, because they can cause reentrancy
* callbacks.
However, this was not enforced. Particularly the latter restriction
is easy to overlook, especially since during the transition period to
the new event system we have bent this rule a couple of times, and the
bad pattern simply involves passing 0 or NULL for the ao_how.
So use the compiler to enforce this property, as follows:
- Mark all functions which take a libxl_asyncop_how, or which
use EGC_INIT or LIBXL__INIT_EGC, with a new annotation
LIBXL_EXTERNAL_CALLERS_ONLY in the public header.
- Change the documentation comment for asynch operations and egcs to
say that this should always be done.
- Arrange that if libxl.h is included via libxl_internal.h,
LIBXL_EXTERNAL_CALLERS_ONLY expands to __attribute__((warning(...))),
which generates a message like this:
libxl.c:1772: warning: call to
''libxl_device_disk_remove''
declared with attribute warning:
may not be called from within libxl
Otherwise, the annotation expands to nothing, so external
callers are unaffected.
- Forbid inclusion of both libxl.h and libxl_internal.h unless
libxl_internal.h came first, so that the above check doesn''t have
any loopholes. Files which include libxl_internal.h should not
include libxl.h as well.
This is enforced explicitly using #error. However, in practice
with the current tree it just changes the error message when this
mistake is made; otherwise we would carry on to immediately
following #define which would cause the compiler to complain that
LIBXL_EXTERNAL_CALLERS_ONLY was redefined. Then the developer
might be tempted to add a #ifndef which would be wrong - it would
leave the affected translation unit unprotected by the new
enforcement regime. So let''s be explicit.
- Fix the one source of files which violate the above principle, the
output from the idl compiler, by removing the redundant inclusion
of libxl.h from the output.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
-
Changes in v2:
* Rebased to current xen-unstable tip.
* Added LIBXL_EXTERNAL_CALLERS_ONLY to more functions.
* -Wno-error is no longer needed as the tree now has no errors of this form.
* check-libxl-api-rules parses #... directives in the preprocessor output
so that it can report errors at their location in libxl.h.
* Indentation in check-libxl-api-rules fixed.
---
.gitignore | 1 +
.hgignore | 1 +
tools/libxl/Makefile | 14 +++++++-
tools/libxl/check-libxl-api-rules | 23 +++++++++++++
tools/libxl/gentypes.py | 1 -
tools/libxl/libxl.h | 64 ++++++++++++++++++++++++++-----------
tools/libxl/libxl_event.h | 21 ++++++++----
tools/libxl/libxl_internal.h | 14 ++++++--
8 files changed, 106 insertions(+), 33 deletions(-)
diff --git a/.gitignore b/.gitignore
index 0891d90..41c89c0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -187,6 +187,7 @@ tools/libxl/xl
tools/libxl/testenum
tools/libxl/testenum.c
tools/libxl/tmp.*
+tools/libxl/libxl.api-for-check
tools/libaio/src/*.ol
tools/libaio/src/*.os
tools/misc/cpuperf/cpuperf-perfcntr
diff --git a/.hgignore b/.hgignore
index 9baf57b..8ff83c7 100644
--- a/.hgignore
+++ b/.hgignore
@@ -185,6 +185,7 @@
^tools/libxl/testidl\.c$
^tools/libxl/tmp\..*$
^tools/libxl/.*\.new$
+^tools/libxl/libxl\.api-for-check
^tools/libvchan/vchan-node[12]$
^tools/libaio/src/.*\.ol$
^tools/libaio/src/.*\.os$
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 424a7ee..395187f 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -74,7 +74,8 @@ LIBXL_OBJS += _libxl_types.o libxl_flask.o
_libxl_types_internal.o
$(LIBXL_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest)
$(CFLAGS_libxenstore) $(CFLAGS_libblktapctl) -include $(XEN_ROOT)/tools/config.h
AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
- _libxl_save_msgs_callout.h _libxl_save_msgs_helper.h
+ _libxl_save_msgs_callout.h _libxl_save_msgs_helper.h \
+ libxl.api-ok
AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
@@ -113,6 +114,15 @@ $(LIBXL_OBJS) $(LIBXLU_OBJS) $(XL_OBJS)
$(SAVE_HELPER_OBJS): $(AUTOINCS)
genpath-target = $(call buildmakevars2file,_paths.h.tmp)
$(eval $(genpath-target))
+libxl.api-ok: check-libxl-api-rules libxl.api-for-check
+ perl $^
+
+%.api-for-check: %.h
+ $(CC) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_$*.o) -c -E $< $(APPEND_CFLAGS) \
+ -DLIBXL_EXTERNAL_CALLERS_ONLY=LIBXL_EXTERNAL_CALLERS_ONLY \
+ >$@.new
+ $(call move-if-changed,$@.new,$@)
+
_paths.h: genpath
sed -e "s/\([^=]*\)=\(.*\)/#define \1 \2/g" $@.tmp >$@.2.tmp
rm -f $@.tmp
@@ -200,7 +210,7 @@ install: all
.PHONY: clean
clean:
$(RM) -f _*.h *.o *.so* *.a $(CLIENTS) $(DEPS)
- $(RM) -f _*.c *.pyc _paths.*.tmp
+ $(RM) -f _*.c *.pyc _paths.*.tmp *.api-for-check
$(RM) -f testidl.c.new testidl.c
# $(RM) -f $(AUTOSRCS) $(AUTOINCS)
diff --git a/tools/libxl/check-libxl-api-rules
b/tools/libxl/check-libxl-api-rules
new file mode 100755
index 0000000..18ff39c
--- /dev/null
+++ b/tools/libxl/check-libxl-api-rules
@@ -0,0 +1,23 @@
+#!/usr/bin/perl -w
+use strict;
+our $needed=0;
+our $speclineoffset=0;
+our $specfile;
+while (<>) {
+ if (m/^\# (\d+) \"(.*)\"$/) {
+ $speclineoffset = $1 - $. -1;
+ $specfile = $2;
+ }
+ my $file = defined($specfile) ? $specfile : $ARGV;
+ my $line = $speclineoffset + $.;
+ if (m/libxl_asyncop_how[^;]/) {
+ $needed=1;
+ }
+ if (m/LIBXL_EXTERNAL_CALLERS_ONLY/) {
+ $needed=0;
+ }
+ next unless $needed;
+ if (m/\;/) {
+ die "$file:$line:missing LIBXL_EXTERNAL_CALLERS_ONLY";
+ }
+}
diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index eb67524..1d13201 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -377,7 +377,6 @@ if __name__ == ''__main__'':
#include <stdlib.h>
#include <string.h>
-#include "libxl.h"
#include "libxl_internal.h"
#define LIBXL_DTOR_POISON 0xa5
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index e1a7296..5ec2d74 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -266,6 +266,13 @@
#endif
#endif
+/* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
+ * called from within libxl itself. Callers outside libxl, who
+ * do not #include libxl_internal.h, are fine. */
+#ifndef LIBXL_EXTERNAL_CALLERS_ONLY
+#define LIBXL_EXTERNAL_CALLERS_ONLY /* disappears for callers outside libxl */
+#endif
+
typedef uint8_t libxl_mac[6];
#define LIBXL_MAC_FMT "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx"
#define LIBXL_MAC_FMTLEN ((2*6)+5) /* 6 hex bytes plus 5 colons */
@@ -496,11 +503,13 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
uint32_t *domid,
const libxl_asyncop_how *ao_how,
- const libxl_asyncprogress_how *aop_console_how);
+ const libxl_asyncprogress_how *aop_console_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
uint32_t *domid, int restore_fd,
const libxl_asyncop_how *ao_how,
- const libxl_asyncprogress_how
*aop_console_how);
+ const libxl_asyncprogress_how *aop_console_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* A progress report will be made via ao_console_how, of type
* domain_create_console_available, when the domain''s primary
* console is available and can be connected to.
@@ -511,7 +520,8 @@ void libxl_domain_config_dispose(libxl_domain_config
*d_config);
int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
int flags, /* LIBXL_SUSPEND_* */
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
#define LIBXL_SUSPEND_DEBUG 1
#define LIBXL_SUSPEND_LIVE 2
@@ -523,12 +533,14 @@ int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid,
int suspend_cancel);
int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
uint32_t domid, int send_fd, int recv_fd,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid);
int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid);
int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid,
libxl_domain_create_info *info, const char *name_suffix, libxl_uuid new_uuid);
/* get max. number of cpus supported by hypervisor */
@@ -549,7 +561,8 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
const char *filename,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t
target_memkb);
int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t
target_memkb, int relative, int enforce);
@@ -677,13 +690,16 @@ void libxl_vcpuinfo_list_free(libxl_vcpuinfo *, int
nr_vcpus);
/* Disks */
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int
*num);
int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
@@ -694,17 +710,21 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t
domid,
* be attached to the guest.
*/
int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* Network Interfaces */
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_nic *nic,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
libxl_device_nic *nic,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int
*num);
int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
@@ -712,23 +732,29 @@ int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t
domid,
/* Keyboard */
int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_vkb *vkb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid,
libxl_device_vkb *vkb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* Framebuffer */
int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_vfb *vfb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid,
libxl_device_vfb *vfb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* PCI Passthrough */
int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci
*pcidev);
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 713d96d..3344bc8 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -37,7 +37,8 @@ typedef int libxl_event_predicate(const libxl_event*, void
*user);
int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
uint64_t typemask,
- libxl_event_predicate *predicate, void *predicate_user);
+ libxl_event_predicate *predicate, void *predicate_user)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* Searches for an event, already-happened, which matches typemask
* and predicate. predicate==0 matches any event.
* libxl_event_check returns the event, which must then later be
@@ -48,7 +49,8 @@ int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_r,
uint64_t typemask,
- libxl_event_predicate *predicate, void *predicate_user);
+ libxl_event_predicate *predicate, void *predicate_user)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* Like libxl_event_check but blocks if no suitable events are
* available, until some are. Uses libxl_osevent_beforepoll/
* _afterpoll so may be inefficient if very many domains are being
@@ -256,7 +258,8 @@ struct pollfd;
*/
int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
struct pollfd *fds, int *timeout_upd,
- struct timeval now);
+ struct timeval now)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* nfds and fds[0..nfds] must be from the most recent call to
* _beforepoll, as modified by poll. (It is therefore not possible
@@ -271,7 +274,8 @@ int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
* libxl_event_check.
*/
void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd
*fds,
- struct timeval now);
+ struct timeval now)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
typedef struct libxl_osevent_hooks {
@@ -357,14 +361,16 @@ void libxl_osevent_register_hooks(libxl_ctx *ctx,
*/
void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
- int fd, short events, short revents);
+ int fd, short events, short revents)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* Implicitly, on entry to this function the timeout has been
* deregistered. If _occurred_timeout is called, libxl will not
* call timeout_deregister; if it wants to requeue the timeout it
* will call timeout_register again.
*/
-void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
+void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/*======================================================================*/
@@ -506,7 +512,8 @@ void libxl_childproc_setmode(libxl_ctx *ctx, const
libxl_childproc_hooks *hooks,
* certainly need to use the self-pipe trick (or a working pselect or
* ppoll) to implement this.
*/
-int libxl_childproc_reaped(libxl_ctx *ctx, pid_t, int status);
+int libxl_childproc_reaped(libxl_ctx *ctx, pid_t, int status)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 691b4f6..58004b3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -52,6 +52,12 @@
#include <xen/io/xenbus.h>
+#ifdef LIBXL_H
+# error libxl.h should be included via libxl_internal.h, not separately
+#endif
+#define LIBXL_EXTERNAL_CALLERS_ONLY \
+ __attribute__((warning("may not be called from within libxl")))
+
#include "libxl.h"
#include "_paths.h"
#include "_libxl_save_msgs_callout.h"
@@ -1534,10 +1540,10 @@ int libxl__hotplug_settings(libxl__gc *gc,
xs_transaction_t t);
*
* Functions using LIBXL__INIT_EGC may *not* generally be called from
* within libxl, because libxl__egc_cleanup may call back into the
- * application. This should be documented near the function
- * prototype(s) for callers of LIBXL__INIT_EGC and EGC_INIT. You
- * should in any case not find it necessary to call egc-creators from
- * within libxl.
+ * application. This should be enforced by declaring all such
+ * functions in libxl.h or libxl_event.h with
+ * LIBXL_EXTERNAL_CALLERS_ONLY. You should in any case not find it
+ * necessary to call egc-creators from within libxl.
*
* The callbacks must all take place with the ctx unlocked because
* the application is entitled to reenter libxl from them. This
--
tg: (a7f0910..) t/xen/xl.external-callers-only (depends on:
t/xen/xl.bootloader.fix.no-blunder-on)
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Campbell
2012-Jul-31 13:12 UTC
Re: [PATCH v2] libxl: enforce prohibitions of internal callers
On Fri, 2012-07-27 at 17:21 +0100, Ian Jackson wrote:> @@ -113,6 +114,15 @@ $(LIBXL_OBJS) $(LIBXLU_OBJS) $(XL_OBJS) $(SAVE_HELPER_OBJS): $(AUTOINCS) > genpath-target = $(call buildmakevars2file,_paths.h.tmp) > $(eval $(genpath-target)) > > +libxl.api-ok: check-libxl-api-rules libxl.api-for-check > + perl $^Someone with a BSD hat is about to say "$(PERL)" here I think ;-) Otherwise is looks good to me: Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian Jackson
2012-Jul-31 14:41 UTC
Re: [PATCH v2] libxl: enforce prohibitions of internal callers
Ian Campbell writes ("Re: [PATCH v2] libxl: enforce prohibitions of
internal callers"):> On Fri, 2012-07-27 at 17:21 +0100, Ian Jackson wrote:
>
> > @@ -113,6 +114,15 @@ $(LIBXL_OBJS) $(LIBXLU_OBJS) $(XL_OBJS)
$(SAVE_HELPER_OBJS): $(AUTOINCS)
> > genpath-target = $(call buildmakevars2file,_paths.h.tmp)
> > $(eval $(genpath-target))
> >
> > +libxl.api-ok: check-libxl-api-rules libxl.api-for-check
> > + perl $^
>
> Someone with a BSD hat is about to say "$(PERL)" here I think ;-)
>
> Otherwise is looks good to me:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Also the commit message should mention the check-libxl-api-rules
script and doesn''t.
I will fix this and resend.
Ian.