| Message ID | 20220330181727.30303-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libxl: Don't segfault on soft-reset failure | expand |
On Wed, Mar 30, 2022 at 02:17:27PM -0400, Jason Andryuk wrote: > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c > index 15ed021f41..631caa416d 100644 > --- a/tools/libs/light/libxl_create.c > +++ b/tools/libs/light/libxl_create.c > @@ -1970,7 +1970,8 @@ static void domcreate_complete(libxl__egc *egc, > libxl_domain_config *const d_config = dcs->guest_config; > libxl_domain_config *d_config_saved = &dcs->guest_config_saved; > > - libxl__xswait_stop(gc, &dcs->console_xswait); > + if (dcs->console_xswait.path) > + libxl__xswait_stop(gc, &dcs->console_xswait); libxl__xswait_stop() needs to be called here. It's a function that can be called several time without ill effect, as long as `console_xswait` is initialised properly (by calling libxl__xswait_init() like you did below. > libxl__domain_build_state_dispose(&dcs->build_state); > > @@ -2176,6 +2177,10 @@ static int do_domain_soft_reset(libxl_ctx *ctx, > aop_console_how); > cdcs->domid_out = &domid_out; > > + /* Initialize in case we end up in domcreate_complete without calling > + * initiate_domain_create. */ That comment isn't needed. This is just part of the initialisation of `cdcs->dcs`. > + libxl__xswait_init(&cdcs->dcs.console_xswait); > + > dom_path = libxl__xs_get_dompath(gc, domid); > if (!dom_path) { > LOGD(ERROR, domid, "failed to read domain path"); As part of the patch, could you also move the libxl__xswait_init() call in initiate_domain_create() to do_domain_create()? That would avoid `console_xswait` been initialised twice, and do_domain_create() is the function that initialise `dcs` before calling initiate_domain_create(). Thanks,
On Thu, Mar 31, 2022 at 10:10 AM Anthony PERARD <anthony.perard@citrix.com> wrote: > > On Wed, Mar 30, 2022 at 02:17:27PM -0400, Jason Andryuk wrote: > > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c > > index 15ed021f41..631caa416d 100644 > > --- a/tools/libs/light/libxl_create.c > > +++ b/tools/libs/light/libxl_create.c > > @@ -1970,7 +1970,8 @@ static void domcreate_complete(libxl__egc *egc, > > libxl_domain_config *const d_config = dcs->guest_config; > > libxl_domain_config *d_config_saved = &dcs->guest_config_saved; > > > > - libxl__xswait_stop(gc, &dcs->console_xswait); > > + if (dcs->console_xswait.path) > > + libxl__xswait_stop(gc, &dcs->console_xswait); > > libxl__xswait_stop() needs to be called here. It's a function that can > be called several time without ill effect, as long as `console_xswait` > is initialised properly (by calling libxl__xswait_init() like you did > below. > > > libxl__domain_build_state_dispose(&dcs->build_state); > > > > @@ -2176,6 +2177,10 @@ static int do_domain_soft_reset(libxl_ctx *ctx, > > aop_console_how); > > cdcs->domid_out = &domid_out; > > > > + /* Initialize in case we end up in domcreate_complete without calling > > + * initiate_domain_create. */ > > That comment isn't needed. This is just part of the initialisation of > `cdcs->dcs`. > > > + libxl__xswait_init(&cdcs->dcs.console_xswait); > > + > > dom_path = libxl__xs_get_dompath(gc, domid); > > if (!dom_path) { > > LOGD(ERROR, domid, "failed to read domain path"); > > > As part of the patch, could you also move the libxl__xswait_init() call > in initiate_domain_create() to do_domain_create()? That would avoid > `console_xswait` been initialised twice, and do_domain_create() is the > function that initialise `dcs` before calling initiate_domain_create(). Sure, that all sounds good. Thanks! Regards, Jason
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index 15ed021f41..631caa416d 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -1970,7 +1970,8 @@ static void domcreate_complete(libxl__egc *egc, libxl_domain_config *const d_config = dcs->guest_config; libxl_domain_config *d_config_saved = &dcs->guest_config_saved; - libxl__xswait_stop(gc, &dcs->console_xswait); + if (dcs->console_xswait.path) + libxl__xswait_stop(gc, &dcs->console_xswait); libxl__domain_build_state_dispose(&dcs->build_state); @@ -2176,6 +2177,10 @@ static int do_domain_soft_reset(libxl_ctx *ctx, aop_console_how); cdcs->domid_out = &domid_out; + /* Initialize in case we end up in domcreate_complete without calling + * initiate_domain_create. */ + libxl__xswait_init(&cdcs->dcs.console_xswait); + dom_path = libxl__xs_get_dompath(gc, domid); if (!dom_path) { LOGD(ERROR, domid, "failed to read domain path");
If domain_soft_reset_cb can't rename the save file, it doesn't call initiate_domain_create() and calls domcreate_complete(). Skipping initiate_domain_create() means dcs->console_wait is uninitialized and all 0s. We have: domcreate_complete() libxl__xswait_stop() libxl__ev_xswatch_deregister(). The uninitialized slotnum 0 is considered valid (-1 is the invalid sentinel), so the NULL pointer path to passed to xs_unwatch() which segfaults. libxl__ev_xswatch_deregister:watch w=0x12bc250 wpath=(null) token=0/0: deregister slotnum=0 Ensure dcs->console_xswait is minimally initialized by calling libxl__xswait_init() in do_domain_soft_reset(). Also add a check for dcs->console_xswait.path being NULL in domcreate_complete() to avoid the segfault. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- The NULL check in domcreate_complete isn't needed when the xswait is initialized, but it could catch other occurances. tools/libs/light/libxl_create.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)