Re: [Xen-devel] [PATCH v1] libxl: fix crash in helper_done due to uninitialized data

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Olaf Hering writes ("[PATCH v1] libxl: fix crash in helper_done due to 
uninitialized data"):
> A crash in helper_done, called from libxl_domain_suspend, was reported,
> triggered by 'virsh migrate --live xen+ssh://host':
...
> This is triggered by a failed poll, the actual error was:
> 
> libxl_aoutils.c:328:datacopier_writable: unexpected poll event 0x1c on fd 37 
> (should be POLLOUT) writing libxc header during copy of save v2 stream
> 
> In this case revents in datacopier_writable is POLLHUP|POLLERR|POLLOUT,
> which triggers datacopier_callback. In helper_done,
> shs->completion_callback is still zero. libxl__xc_domain_save fills
> dss.sws.shs. But that function is only called after stream_header_done.
> Any error before that will leave dss partly uninitialized.

Thanks for the diagnosis.  And sorry for the inconvenience of this
bug.

> Fix this crash by checking if ->completion_callback is valid.

But I don't think this fix is right.  The bug is that
libxl__save_helper_abort is called on a blank shs.  (You say
"uninitialised" but actually this is all zeroed at some point, so it
it's not reading uninitialised memory.)

I think it is in fact a bug that this error path

    if (!stream->rc && rc) {
        /* First reported failure. Tear everything down. */
        stream->rc = rc;
        stream->sync_teardown = true;

        libxl__stream_read_abort(egc, stream, rc);
        libxl__save_helper_abort(egc, &stream->shs);
        libxl__conversion_helper_abort(egc, &stream->chs, rc);

        stream->sync_teardown = false;
    }

calls libxl__save_helper_abort when it hasn't ever called anything
other than libxl__save_helper_init.  Because _abort naturally wants to
call the failure callback.

I suggest that we add a check for _inuse to this bit of
check_all_finished.

Ross and Andrew, you wrote much of this stream read stuff, what do you
think ?

Part of the difficulty is that the possible states and transitions of
a libxl__save_helper_state are not formatlly documented.  That's my
fault - sorry.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel