On Wed, Apr 23, 2014 at 03:31:36PM +0200, Sebastian Krahmer wrote:
Please put the patch revision number in the tags section of the commit
message. This way git-am(1) automatically strips it when applying the
patch. So:
[PATCH] ivshmem: fix potential OOB r/w access (#2)
should be:
[PATCH v2] ivshmem: fix potential OOB r/w access
The reason for this is to reduce noise in the commit history. Revision
numbers only matter during code review.
> tmp_fd does not leak on error; see following dup() call.
> According to docu g_realloc() may return NULL so we need
> to check that. Passes checkpatch.pl, after also fixing wrong
> ivshmem.c style itself.
These are responses to my review comments. They should not be part of
the commit description. Please place changelogs or comments like this
underneath the '---' line. Again, it's a convention for reducing noise
in the commit history.
Addressing your response:
> tmp_fd does not leak on error; see following dup() call
The existing ivshmem code is broken and leaks the file descriptor.
Please see tcp_get_msgfd(). It transfers ownership of the file
descriptor to the caller.
For more evidence, see the qmp_add_fd() call site.
I just noticed that another caller, qmp_getfd() leaks the fd in an error
code path and am sending a patch to fix that.
Since you are modifying ivshmem, please fix the fd leak in ivshmem and
include a patch in this series.
> According to docu g_realloc() may return NULL so we need to check
> that.
There is only one case where it returns NULL and we cannot reach it.
The documentation says: "n_bytes may be 0, in which case NULL will be
returned". We never pass n_bytes 0 because of the new_min_size <= 0
check added in your patch.
The entire glib memory allocator is designed to abort with a fatal error
if malloc(3)/realloc(3) fail to allocate memory. The documentation
states that here:
"If any call to allocate memory fails, the application is terminated.
This also means that there is no need to check if the call succeeded."
Here is the g_realloc() implementation from glib:
newmem = glib_mem_vtable.realloc (mem, n_bytes);
TRACE (GLIB_MEM_REALLOC((void*) newmem, (void*)mem, (unsigned int) n_bytes,
0));
if (newmem)
return newmem;
g_error ("%s: failed to allocate %"G_GSIZE_FORMAT" bytes",
G_STRLOC, n_bytes);
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 8d144ba..5c0f116 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -28,6 +28,7 @@
>
> #include <sys/mman.h>
> #include <sys/types.h>
> +#include <limits.h>
>
> #define PCI_VENDOR_ID_IVSHMEM PCI_VENDOR_ID_REDHAT_QUMRANET
> #define PCI_DEVICE_ID_IVSHMEM 0x1110
> @@ -401,23 +402,41 @@ static void close_guest_eventfds(IVShmemState *s, int
> posn)
>
> /* this function increase the dynamic storage need to store data about other
> * guests */
> -static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
> +static int increase_dynamic_storage(IVShmemState *s, int new_min_size)
> +{
>
> int j, old_nb_alloc;
>
> + /* check for integer overflow */
> + if (new_min_size >= INT_MAX/sizeof(Peer) - 1 || new_min_size <= 0) {
> + return -1;
> + }
> +
> old_nb_alloc = s->nb_peers;
>
> - while (new_min_size >= s->nb_peers)
> - s->nb_peers = s->nb_peers * 2;
> + /* heap allocators already have good alloc strategies, no need
> + * to re-implement here. +1 because #new_min_size is used as last array
> + * index */
This comment is confusing since it refers to code that is deleted by
your patch. People reading the code will not understand why it mentions
re-implementing heap allocation strategies - the loop will be gone.
Please drop this part of the comment.
