[Qemu-commits] [qemu/qemu] ba42eb: 9pfs: allocate space for guest originated empty st...

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: ba42ebb863ab7d40adc79298422ed9596df8f73a
      
https://github.com/qemu/qemu/commit/ba42ebb863ab7d40adc79298422ed9596df8f73a
  Author: Li Qiang <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M fsdev/9p-iov-marshal.c
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: allocate space for guest originated empty strings

If a guest sends an empty string paramater to any 9P operation, the current
code unmarshals it into a V9fsString equal to { .size = 0, .data = NULL }.

This is unfortunate because it can cause NULL pointer dereference to happen
at various locations in the 9pfs code. And we don't want to check str->data
everywhere we pass it to strcmp() or any other function which expects a
dereferenceable pointer.

This patch enforces the allocation of genuine C empty strings instead, so
callers don't have to bother.

Out of all v9fs_iov_vunmarshal() users, only v9fs_xattrwalk() checks if
the returned string is empty. It now uses v9fs_string_size() since
name.data cannot be NULL anymore.

Signed-off-by: Li Qiang <address@hidden>
[groug, rewritten title and changelog,
 fix empty string check in v9fs_xattrwalk()]
Signed-off-by: Greg Kurz <address@hidden>


  Commit: e95c9a493a5a8d6f969e86c9f19f80ffe6587e19
      
https://github.com/qemu/qemu/commit/e95c9a493a5a8d6f969e86c9f19f80ffe6587e19
  Author: Li Qiang <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: fix potential host memory leak in v9fs_read

In 9pfs read dispatch function, it doesn't free two QEMUIOVector
object thus causing potential memory leak. This patch avoid this.

Signed-off-by: Li Qiang <address@hidden>
Signed-off-by: Greg Kurz <address@hidden>


  Commit: bc70a5925f1928623b1fcc033f772daa0d0d271f
      
https://github.com/qemu/qemu/commit/bc70a5925f1928623b1fcc033f772daa0d0d271f
  Author: Greg Kurz <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M fsdev/9p-marshal.h
    M hw/9pfs/9p-synth.h
    M hw/9pfs/9p.h
    M hw/9pfs/coth.h
    M hw/9pfs/virtio-9p.h

  Log Message:
  -----------
  9pfs: fsdev: drop useless extern annotation for functions

Signed-off-by: Greg Kurz <address@hidden>


  Commit: 5bdade66211c8023d8e81c535f4944cbf830b25a
      
https://github.com/qemu/qemu/commit/5bdade66211c8023d8e81c535f4944cbf830b25a
  Author: Greg Kurz <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/codir.c
    M hw/9pfs/cofile.c
    M hw/9pfs/cofs.c
    M hw/9pfs/coth.h
    M hw/9pfs/coxattr.c

  Log Message:
  -----------
  9pfs: use coroutine_fn annotation in hw/9pfs/co*.[ch]

All these functions use the v9fs_co_run_in_worker() macro, and thus always
call qemu_coroutine_self() and qemu_coroutine_yield().

Let's mark them to make it obvious they execute in coroutine context.

Signed-off-by: Greg Kurz <address@hidden>


  Commit: 8440e22ec1a5deabc4fcf5c4826d5c73ddc15765
      
https://github.com/qemu/qemu/commit/8440e22ec1a5deabc4fcf5c4826d5c73ddc15765
  Author: Greg Kurz <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c
    M hw/9pfs/9p.h

  Log Message:
  -----------
  9pfs: use coroutine_fn annotation in hw/9pfs/9p.[ch]

All these functions either call the v9fs_co_* functions which have the
coroutine_fn annotation, or pdu_complete() which calls qemu_co_queue_next().

Let's mark them to make it obvious they execute in coroutine context.

Signed-off-by: Greg Kurz <address@hidden>


  Commit: 6868a420c519d74926ea814d48f6ce9beda35b98
      
https://github.com/qemu/qemu/commit/6868a420c519d74926ea814d48f6ce9beda35b98
  Author: Greg Kurz <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: drop useless check in pdu_free()

Out of the three users of pdu_free(), none ever passes a NULL pointer to
this function.

Signed-off-by: Greg Kurz <address@hidden>


  Commit: f74e27bf0f07425aba6cb812aa7f5aa98bb68542
      
https://github.com/qemu/qemu/commit/f74e27bf0f07425aba6cb812aa7f5aa98bb68542
  Author: Greg Kurz <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: only free completed request if not flushed

If a PDU has a flush request pending, the current code calls pdu_free()
twice:

1) pdu_complete()->pdu_free() with pdu->cancelled set, which does nothing

2) v9fs_flush()->pdu_free() with pdu->cancelled cleared, which moves the
   PDU back to the free list.

This works but it complexifies the logic of pdu_free().

With this patch, pdu_complete() only calls pdu_free() if no flush request
is pending, i.e. qemu_co_queue_next() returns false.

Since pdu_free() is now supposed to be called with pdu->cancelled cleared,
the check in pdu_free() is dropped and replaced by an assertion.

Signed-off-by: Greg Kurz <address@hidden>


  Commit: 0e44a0fd3f28cccb8963fdfc05c53c546b3f46b6
      
https://github.com/qemu/qemu/commit/0e44a0fd3f28cccb8963fdfc05c53c546b3f46b6
  Author: Greg Kurz <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c
    M hw/9pfs/9p.h
    M hw/9pfs/virtio-9p-device.c

  Log Message:
  -----------
  virtio-9p: add reset handler

Virtio devices should implement the VirtIODevice->reset() function to
perform necessary cleanup actions and to bring the device to a quiescent
state.

In the case of the virtio-9p device, this means:
- emptying the list of active PDUs (i.e. draining all in-flight I/O)
- freeing all fids (i.e. close open file descriptors and free memory)

That's what this patch does.

The reset handler first waits for all active PDUs to complete. Since
completion happens in the QEMU global aio context, we just have to
loop around aio_poll() until the active list is empty.

The freeing part involves some actions to be performed on the backend,
like closing file descriptors or flushing extended attributes to the
underlying filesystem. The virtfs_reset() function already does the
job: it calls free_fid() for all open fids not involved in an ongoing
I/O operation. We are sure this is the case since we have drained
the PDU active list.

The current code implements all backend accesses with coroutines, but we
want to stay synchronous on the reset path. We can either change the
current code to be able to run when not in coroutine context, or create
a coroutine context and wait for virtfs_reset() to complete. This patch
goes for the latter because it results in simpler code.

Note that we also need to create a dummy PDU because it is also an API
to pass the FsContext pointer to all backend callbacks.

Signed-off-by: Greg Kurz <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>


  Commit: eb687602853b4ae656e9236ee4222609f3a6887d
      
https://github.com/qemu/qemu/commit/eb687602853b4ae656e9236ee4222609f3a6887d
  Author: Li Qiang <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: fix information leak in xattr read

9pfs uses g_malloc() to allocate the xattr memory space, if the guest
reads this memory before writing to it, this will leak host heap memory
to the guest. This patch avoid this.

Signed-off-by: Li Qiang <address@hidden>
Reviewed-by: Greg Kurz <address@hidden>
Signed-off-by: Greg Kurz <address@hidden>


  Commit: ff55e94d23ae94c8628b0115320157c763eb3e06
      
https://github.com/qemu/qemu/commit/ff55e94d23ae94c8628b0115320157c763eb3e06
  Author: Li Qiang <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: fix memory leak in v9fs_xattrcreate

The 'fs.xattr.value' field in V9fsFidState object doesn't consider the
situation that this field has been allocated previously. Every time, it
will be allocated directly. This leads to a host memory leak issue if
the client sends another Txattrcreate message with the same fid number
before the fid from the previous time got clunked.

Signed-off-by: Li Qiang <address@hidden>
Reviewed-by: Greg Kurz <address@hidden>
[groug, updated the changelog to indicate how the leak can occur]
Signed-off-by: Greg Kurz <address@hidden>


  Commit: 4c1586787ff43c9acd18a56c12d720e3e6be9f7c
      
https://github.com/qemu/qemu/commit/4c1586787ff43c9acd18a56c12d720e3e6be9f7c
  Author: Li Qiang <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: fix memory leak in v9fs_link

The v9fs_link() function keeps a reference on the source fid object. This
causes a memory leak since the reference never goes down to 0. This patch
fixes the issue.

Signed-off-by: Li Qiang <address@hidden>
Reviewed-by: Greg Kurz <address@hidden>
[groug, rephrased the changelog]
Signed-off-by: Greg Kurz <address@hidden>


  Commit: fdfcc9aeea1492f4b819a24c94dfb678145b1bf9
      
https://github.com/qemu/qemu/commit/fdfcc9aeea1492f4b819a24c94dfb678145b1bf9
  Author: Li Qiang <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M hw/9pfs/9p.c

  Log Message:
  -----------
  9pfs: fix memory leak in v9fs_write

If an error occurs when marshalling the transfer length to the guest, the
v9fs_write() function doesn't free an IO vector, thus leading to a memory
leak. This patch fixes the issue.

Signed-off-by: Li Qiang <address@hidden>
Reviewed-by: Greg Kurz <address@hidden>
[groug, rephrased the changelog]
Signed-off-by: Greg Kurz <address@hidden>


  Commit: 0975b8b823a888d474fa33821dfe84e6904db197
      
https://github.com/qemu/qemu/commit/0975b8b823a888d474fa33821dfe84e6904db197
  Author: Peter Maydell <address@hidden>
  Date:   2016-10-17 (Mon, 17 Oct 2016)

  Changed paths:
    M fsdev/9p-iov-marshal.c
    M fsdev/9p-marshal.h
    M hw/9pfs/9p-synth.h
    M hw/9pfs/9p.c
    M hw/9pfs/9p.h
    M hw/9pfs/codir.c
    M hw/9pfs/cofile.c
    M hw/9pfs/cofs.c
    M hw/9pfs/coth.h
    M hw/9pfs/coxattr.c
    M hw/9pfs/virtio-9p-device.c
    M hw/9pfs/virtio-9p.h

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/gkurz/tags/for-upstream' into staging

This pull request contains:
- a patch to add a vdc->reset() handler to virtio-9p
- a bunch of patches to fix various memory leaks (thanks to Li Qiang)
- some code cleanups for 9pfs

# gpg: Signature made Mon 17 Oct 2016 16:01:46 BST
# gpg:                using DSA key 0x02FC3AEB0101DBC2
# gpg: Good signature from "Greg Kurz <address@hidden>"
# gpg:                 aka "Greg Kurz <address@hidden>"
# gpg:                 aka "Greg Kurz <address@hidden>"
# gpg:                 aka "Greg Kurz <address@hidden>"
# gpg:                 aka "Gregory Kurz (Groug) <address@hidden>"
# gpg:                 aka "Gregory Kurz (Cimai Technology) <address@hidden>"
# gpg:                 aka "Gregory Kurz (Meiosys Technology) <address@hidden>"
# gpg: WARNING: This key is not certified with a trusted signature!
# gpg:          There is no indication that the signature belongs to the owner.
# Primary key fingerprint: 2BD4 3B44 535E C0A7 9894  DBA2 02FC 3AEB 0101 DBC2

* remotes/gkurz/tags/for-upstream:
  9pfs: fix memory leak in v9fs_write
  9pfs: fix memory leak in v9fs_link
  9pfs: fix memory leak in v9fs_xattrcreate
  9pfs: fix information leak in xattr read
  virtio-9p: add reset handler
  9pfs: only free completed request if not flushed
  9pfs: drop useless check in pdu_free()
  9pfs: use coroutine_fn annotation in hw/9pfs/9p.[ch]
  9pfs: use coroutine_fn annotation in hw/9pfs/co*.[ch]
  9pfs: fsdev: drop useless extern annotation for functions
  9pfs: fix potential host memory leak in v9fs_read
  9pfs: allocate space for guest originated empty strings

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/7bf59dfec423...0975b8b823a8