On 06/06/2013 05:22 AM, Stefan Hajnoczi wrote:
On Wed, Jun 05, 2013 at 04:47:59PM -0400, Corey Bryant wrote:+/* + * Coroutine that reads a blob from the drive asynchronously + */ +static void coroutine_fn tpm_nvram_co_read(void *opaque) +{ + TPMNvramRWRequest *rwr = opaque; + + *rwr->blob_r = g_malloc(rwr->size); + + rwr->rc = bdrv_pread(rwr->bdrv, + rwr->offset, + *rwr->blob_r, + rwr->size); + if (rwr->rc != rwr->size) { + g_free(*rwr->blob_r); + *rwr->blob_r = NULL; + } + + qemu_mutex_lock(&rwr->completion_mutex);Race condition: we must only store rwr->rc while holding ->completion_mutex. Otherwise the other thread may see ->rc and g_free(rwr) before we leave this function, causing us to operate on freed memory. I suggest storing rc into a local variable first and then assigning to rwr->rc here.+/* + * Enter a coroutine to write a blob to the drive + */ +static void tpm_nvram_do_co_write(TPMNvramRWRequest *rwr) +{ + int rc; + Coroutine *co; + + rc = tpm_nvram_adjust_size(rwr->bdrv, rwr->offset, rwr->size); + if (rc < 0) { + rwr->rc = rc; + return; + }Do this inside the tpm_nvram_co_write() coroutine so error return still signals the condvar. Right now the other thread may miss completion and deadlock.
Thanks for the review. Sending a v3 out to the list now. -- Regards, Corey Bryant
