Control: tags -1 + patch
Attaching as well the two proposed patches (and which make the
testcase pass).
Regards,
Salvatore
>From 78b54cbaa064f0ac94af114edb54fca3b365430d Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fzig...@redhat.com>
Date: Fri, 19 Jun 2015 14:42:54 +0100
Subject: [PATCH spice-common 1/3] Write a small test to test possible crash
This small test prove a that current generated demarshaller code
is not safe to integer overflows leading to buffer overflows.
Actually from a quick look at the protocol it seems that client
can't cause these overflows but server can quite easily at
demonstrated by this test.
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
tests/Makefile.am | 14 +++++++++
tests/test-overflow.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 94 insertions(+)
create mode 100644 tests/test-overflow.c
Index: spice-gtk-0.34/spice-common/tests/Makefile.am
===================================================================
--- spice-gtk-0.34.orig/spice-common/tests/Makefile.am
+++ spice-gtk-0.34/spice-common/tests/Makefile.am
@@ -63,4 +63,18 @@ EXTRA_DIST = \
test-marshallers.proto \
$(NULL)
+TESTS += test_overflow
+test_overflow_SOURCES = test-overflow.c
+test_overflow_CFLAGS = \
+ -I$(top_srcdir) \
+ $(GLIB2_CFLAGS) \
+ $(SPICE_COMMON_CFLAGS) \
+ $(PROTOCOL_CFLAGS) \
+ $(NULL)
+test_overflow_LDADD = \
+ $(top_builddir)/common/libspice-common.la \
+ $(top_builddir)/common/libspice-common-server.la \
+ $(top_builddir)/common/libspice-common-client.la \
+ $(NULL)
+
-include $(top_srcdir)/git.mk
Index: spice-gtk-0.34/spice-common/tests/test-overflow.c
===================================================================
--- /dev/null
+++ spice-gtk-0.34/spice-common/tests/test-overflow.c
@@ -0,0 +1,80 @@
+/*
+ Copyright (C) 2015 Red Hat, Inc.
+
+ This library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ This library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+
+#include <common/marshaller.h>
+#include <common/generated_server_marshallers.h>
+#include <common/client_demarshallers.h>
+
+#define NUM_CHANNELS 3u
+
+int main(void)
+{
+ SpiceMarshaller *m;
+ SpiceMsgChannels *msg;
+ uint8_t *data, *out;
+ size_t len;
+ int to_free = 0;
+ spice_parse_channel_func_t func;
+ unsigned int max_message_type, n;
+ message_destructor_t free_output;
+
+ m = spice_marshaller_new();
+ assert(m);
+
+ msg = (SpiceMsgChannels *) malloc(sizeof(SpiceMsgChannels) +
+ NUM_CHANNELS * sizeof(SpiceChannelId));
+ assert(msg);
+
+ // build a message and marshal it
+ msg->num_of_channels = NUM_CHANNELS;
+ for (n = 0; n < NUM_CHANNELS; ++n)
+ msg->channels[n] = (SpiceChannelId) { n + 1, n * 7 };
+ spice_marshall_msg_main_channels_list(m, msg);
+
+ // get linear data
+ data = spice_marshaller_linearize(m, 0, &len, &to_free);
+ assert(data);
+
+ printf("output len %lu\n", (unsigned long) len);
+
+ // hack, try to core
+ *((uint32_t *) data) = 0x80000002u;
+
+ // extract the message
+ func = spice_get_server_channel_parser(1, &max_message_type);
+ assert(func);
+ out = func(data, data+len, SPICE_MSG_MAIN_CHANNELS_LIST, 0, &len, &free_output);
+ assert(out == NULL);
+
+ // cleanup
+ if (to_free)
+ free(data);
+ if (out)
+ free_output(out);
+ free(msg);
+
+ return 0;
+}
+
Index: spice-gtk-0.34/spice-common/tests/Makefile.in
===================================================================
--- spice-gtk-0.34.orig/spice-common/tests/Makefile.in
+++ spice-gtk-0.34/spice-common/tests/Makefile.in
@@ -1,7 +1,7 @@
-# Makefile.in generated by automake 1.15 from Makefile.am.
+# Makefile.in generated by automake 1.15.1 from Makefile.am.
# @configure_input@
-# Copyright (C) 1994-2014 Free Software Foundation, Inc.
+# Copyright (C) 1994-2017 Free Software Foundation, Inc.
# This Makefile.in is free software; the Free Software Foundation
# gives unlimited permission to copy and/or distribute it,
@@ -88,7 +88,8 @@ PRE_UNINSTALL = :
POST_UNINSTALL = :
build_triplet = @build@
host_triplet = @host@
-TESTS = test_logging$(EXEEXT) test_marshallers$(EXEEXT)
+TESTS = test_logging$(EXEEXT) test_marshallers$(EXEEXT) \
+ test_overflow$(EXEEXT)
noinst_PROGRAMS = $(am__EXEEXT_1)
subdir = tests
ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
@@ -104,7 +105,8 @@ mkinstalldirs = $(install_sh) -d
CONFIG_HEADER = $(top_builddir)/config.h
CONFIG_CLEAN_FILES =
CONFIG_CLEAN_VPATH_FILES =
-am__EXEEXT_1 = test_logging$(EXEEXT) test_marshallers$(EXEEXT)
+am__EXEEXT_1 = test_logging$(EXEEXT) test_marshallers$(EXEEXT) \
+ test_overflow$(EXEEXT)
PROGRAMS = $(noinst_PROGRAMS)
am_test_logging_OBJECTS = test_logging-test-logging.$(OBJEXT)
test_logging_OBJECTS = $(am_test_logging_OBJECTS)
@@ -130,6 +132,16 @@ test_marshallers_LINK = $(LIBTOOL) $(AM_
$(AM_LIBTOOLFLAGS) $(LIBTOOLFLAGS) --mode=link $(CCLD) \
$(test_marshallers_CFLAGS) $(CFLAGS) $(AM_LDFLAGS) $(LDFLAGS) \
-o $@
+am_test_overflow_OBJECTS = test_overflow-test-overflow.$(OBJEXT)
+test_overflow_OBJECTS = $(am_test_overflow_OBJECTS)
+test_overflow_DEPENDENCIES = \
+ $(top_builddir)/common/libspice-common.la \
+ $(top_builddir)/common/libspice-common-server.la \
+ $(top_builddir)/common/libspice-common-client.la \
+ $(am__DEPENDENCIES_1)
+test_overflow_LINK = $(LIBTOOL) $(AM_V_lt) --tag=CC $(AM_LIBTOOLFLAGS) \
+ $(LIBTOOLFLAGS) --mode=link $(CCLD) $(test_overflow_CFLAGS) \
+ $(CFLAGS) $(AM_LDFLAGS) $(LDFLAGS) -o $@
AM_V_P = $(am__v_P_@AM_V@)
am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
am__v_P_0 = false
@@ -164,8 +176,10 @@ AM_V_CCLD = $(am__v_CCLD_@AM_V@)
am__v_CCLD_ = $(am__v_CCLD_@AM_DEFAULT_V@)
am__v_CCLD_0 = @echo " CCLD " $@;
am__v_CCLD_1 =
-SOURCES = $(test_logging_SOURCES) $(test_marshallers_SOURCES)
-DIST_SOURCES = $(test_logging_SOURCES) $(test_marshallers_SOURCES)
+SOURCES = $(test_logging_SOURCES) $(test_marshallers_SOURCES) \
+ $(test_overflow_SOURCES)
+DIST_SOURCES = $(test_logging_SOURCES) $(test_marshallers_SOURCES) \
+ $(test_overflow_SOURCES)
am__can_run_installinfo = \
case $$AM_UPDATE_INFO_DIR in \
n|no|NO) false;; \
@@ -537,6 +551,7 @@ program_transform_name = @program_transf
psdir = @psdir@
pyexecdir = @pyexecdir@
pythondir = @pythondir@
+runstatedir = @runstatedir@
sbindir = @sbindir@
sharedstatedir = @sharedstatedir@
srcdir = @srcdir@
@@ -601,6 +616,20 @@ EXTRA_DIST = \
test-marshallers.proto \
$(NULL)
+test_overflow_SOURCES = test-overflow.c
+test_overflow_CFLAGS = \
+ -I$(top_srcdir) \
+ $(GLIB2_CFLAGS) \
+ $(SPICE_COMMON_CFLAGS) \
+ $(PROTOCOL_CFLAGS) \
+ $(NULL)
+
+test_overflow_LDADD = \
+ $(top_builddir)/common/libspice-common.la \
+ $(top_builddir)/common/libspice-common-server.la \
+ $(top_builddir)/common/libspice-common-client.la \
+ $(NULL)
+
all: $(BUILT_SOURCES)
$(MAKE) $(AM_MAKEFLAGS) all-am
@@ -653,6 +682,10 @@ test_marshallers$(EXEEXT): $(test_marsha
@rm -f test_marshallers$(EXEEXT)
$(AM_V_CCLD)$(test_marshallers_LINK) $(test_marshallers_OBJECTS) $(test_marshallers_LDADD) $(LIBS)
+test_overflow$(EXEEXT): $(test_overflow_OBJECTS) $(test_overflow_DEPENDENCIES) $(EXTRA_test_overflow_DEPENDENCIES)
+ @rm -f test_overflow$(EXEEXT)
+ $(AM_V_CCLD)$(test_overflow_LINK) $(test_overflow_OBJECTS) $(test_overflow_LDADD) $(LIBS)
+
mostlyclean-compile:
-rm -f *.$(OBJEXT)
@@ -662,6 +695,7 @@ distclean-compile:
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/test_logging-test-logging.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/test_marshallers-generated_test_marshallers.Po@am__quote@
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/test_marshallers-test-marshallers.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/test_overflow-test-overflow.Po@am__quote@
.c.o:
@am__fastdepCC_TRUE@ $(AM_V_CC)$(COMPILE) -MT $@ -MD -MP -MF $(DEPDIR)/$*.Tpo -c -o $@ $<
@@ -726,6 +760,20 @@ test_marshallers-test-marshallers.obj: t
@AMDEP_TRUE@@am__fastdepCC_FALSE@ DEPDIR=$(DEPDIR) $(CCDEPMODE) $(depcomp) @AMDEPBACKSLASH@
@am__fastdepCC_FALSE@ $(AM_V_CC@am__nodep@)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(test_marshallers_CFLAGS) $(CFLAGS) -c -o test_marshallers-test-marshallers.obj `if test -f 'test-marshallers.c'; then $(CYGPATH_W) 'test-marshallers.c'; else $(CYGPATH_W) '$(srcdir)/test-marshallers.c'; fi`
+test_overflow-test-overflow.o: test-overflow.c
+@am__fastdepCC_TRUE@ $(AM_V_CC)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(test_overflow_CFLAGS) $(CFLAGS) -MT test_overflow-test-overflow.o -MD -MP -MF $(DEPDIR)/test_overflow-test-overflow.Tpo -c -o test_overflow-test-overflow.o `test -f 'test-overflow.c' || echo '$(srcdir)/'`test-overflow.c
+@am__fastdepCC_TRUE@ $(AM_V_at)$(am__mv) $(DEPDIR)/test_overflow-test-overflow.Tpo $(DEPDIR)/test_overflow-test-overflow.Po
+@AMDEP_TRUE@@am__fastdepCC_FALSE@ $(AM_V_CC)source='test-overflow.c' object='test_overflow-test-overflow.o' libtool=no @AMDEPBACKSLASH@
+@AMDEP_TRUE@@am__fastdepCC_FALSE@ DEPDIR=$(DEPDIR) $(CCDEPMODE) $(depcomp) @AMDEPBACKSLASH@
+@am__fastdepCC_FALSE@ $(AM_V_CC@am__nodep@)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(test_overflow_CFLAGS) $(CFLAGS) -c -o test_overflow-test-overflow.o `test -f 'test-overflow.c' || echo '$(srcdir)/'`test-overflow.c
+
+test_overflow-test-overflow.obj: test-overflow.c
+@am__fastdepCC_TRUE@ $(AM_V_CC)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(test_overflow_CFLAGS) $(CFLAGS) -MT test_overflow-test-overflow.obj -MD -MP -MF $(DEPDIR)/test_overflow-test-overflow.Tpo -c -o test_overflow-test-overflow.obj `if test -f 'test-overflow.c'; then $(CYGPATH_W) 'test-overflow.c'; else $(CYGPATH_W) '$(srcdir)/test-overflow.c'; fi`
+@am__fastdepCC_TRUE@ $(AM_V_at)$(am__mv) $(DEPDIR)/test_overflow-test-overflow.Tpo $(DEPDIR)/test_overflow-test-overflow.Po
+@AMDEP_TRUE@@am__fastdepCC_FALSE@ $(AM_V_CC)source='test-overflow.c' object='test_overflow-test-overflow.obj' libtool=no @AMDEPBACKSLASH@
+@AMDEP_TRUE@@am__fastdepCC_FALSE@ DEPDIR=$(DEPDIR) $(CCDEPMODE) $(depcomp) @AMDEPBACKSLASH@
+@am__fastdepCC_FALSE@ $(AM_V_CC@am__nodep@)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(test_overflow_CFLAGS) $(CFLAGS) -c -o test_overflow-test-overflow.obj `if test -f 'test-overflow.c'; then $(CYGPATH_W) 'test-overflow.c'; else $(CYGPATH_W) '$(srcdir)/test-overflow.c'; fi`
+
mostlyclean-libtool:
-rm -f *.lo
@@ -938,6 +986,13 @@ test_marshallers.log: test_marshallers$(
$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
--log-file $$b.log --trs-file $$b.trs \
$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+ "$$tst" $(AM_TESTS_FD_REDIRECT)
+test_overflow.log: test_overflow$(EXEEXT)
+ @p='test_overflow$(EXEEXT)'; \
+ b='test_overflow'; \
+ $(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+ --log-file $$b.log --trs-file $$b.trs \
+ $(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
"$$tst" $(AM_TESTS_FD_REDIRECT)
.test.log:
@p='$<'; \
>From f79349debc27e32e098ef196b8f08486cc33c835 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fzig...@redhat.com>
Date: Fri, 19 Jun 2015 14:44:37 +0100
Subject: [PATCH spice-common 2/3] Fix integer overflows computing sizes
Make code safe using both 32 and 64 bit machine.
Consider that this code can be compiled for machines with 32 bit.
There are some arrays length which are 32 bit.
If size_t this can cause easily an overflow. For instance message_len
sending SPICE_MSG_NOTIFY messages are 32 bit and code add a small
constant (currently 24) before doing the test for size. Now passing
(uint32_t) -20 as message_len would lead to a size of 4 after the
addition. This overflow does not happen on 64 bit machine as the length
is converted to size_t.
There are also some array length where some item are bigger than 1 byte.
For instance SPICE_MAIN_CHANNELS_LIST message have a number of channels
and each channel is composed by 2 bytes. Now the code generated try to do
length * 2 where length is still a 32 bit so if we put a value like
0x80000002u we get 4 as length. This will cause an overflow as code will
allocate very few bytes but try to fill with a huge number of elements.
This overflow happen in both 32 and 64 bit machine.
To avoid all these possible overflows this patch use only 64 bit for
nelements (number of elements), nw_size (network size) and mem_size
(memory size needed) checking the sizes to avoid other overflows
(like pointers conversions under 32 bit machines).
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
python_modules/demarshal.py | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
--- a/spice-common/python_modules/demarshal.py
+++ b/spice-common/python_modules/demarshal.py
@@ -88,7 +88,7 @@ def write_parser_helpers(writer):
writer.variable_def("uint64_t", "offset")
writer.variable_def("parse_func_t", "parse")
writer.variable_def("void **", "dest")
- writer.variable_def("uint32_t", "nelements")
+ writer.variable_def("uint64_t", "nelements")
writer.end_block(semicolon=True)
def write_read_primitive(writer, start, container, name, scope):
@@ -173,7 +173,7 @@ def write_validate_switch_member(writer,
all_as_extra_size = m.is_extra_size() and want_extra_size
if not want_mem_size and all_as_extra_size and not scope.variable_defined(item.mem_size()):
- scope.variable_def("uint32_t", item.mem_size())
+ scope.variable_def("uint64_t", item.mem_size())
sub_want_mem_size = want_mem_size or all_as_extra_size
sub_want_extra_size = want_extra_size and not all_as_extra_size
@@ -206,7 +206,7 @@ def write_validate_struct_function(write
scope = writer.function(validate_function, "static intptr_t", "uint8_t *message_start, uint8_t *message_end, uint64_t offset, SPICE_GNUC_UNUSED int minor")
scope.variable_def("uint8_t *", "start = message_start + offset")
scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
- scope.variable_def("size_t", "mem_size", "nw_size")
+ scope.variable_def("uint64_t", "mem_size", "nw_size")
num_pointers = struct.get_num_pointers()
if num_pointers != 0:
scope.variable_def("SPICE_GNUC_UNUSED intptr_t", "ptr_size")
@@ -223,7 +223,7 @@ def write_validate_struct_function(write
writer.newline()
writer.comment("Check if struct fits in reported side").newline()
- writer.error_check("start + nw_size > message_end")
+ writer.error_check("nw_size > (uintptr_t) (message_end - start)")
writer.statement("return mem_size")
@@ -251,26 +251,26 @@ def write_validate_pointer_item(writer,
# if array, need no function check
if target_type.is_array():
- writer.error_check("message_start + %s >= message_end" % v)
+ writer.error_check("%s >= (uintptr_t) (message_end - message_start)" % v)
assert target_type.element_type.is_primitive()
array_item = ItemInfo(target_type, "%s__array" % item.prefix, start)
- scope.variable_def("uint32_t", array_item.nw_size())
+ scope.variable_def("uint64_t", array_item.nw_size())
# don't create a variable that isn't used, fixes -Werror=unused-but-set-variable
need_mem_size = want_mem_size or (
want_extra_size and not item.member.has_attr("chunk")
and not target_type.is_cstring_length())
if need_mem_size:
- scope.variable_def("uint32_t", array_item.mem_size())
+ scope.variable_def("uint64_t", array_item.mem_size())
if target_type.is_cstring_length():
writer.assign(array_item.nw_size(), "spice_strnlen((char *)message_start + %s, message_end - (message_start + %s))" % (v, v))
writer.error_check("*(message_start + %s + %s) != 0" % (v, array_item.nw_size()))
else:
write_validate_array_item(writer, container, array_item, scope, parent_scope, start,
True, want_mem_size=need_mem_size, want_extra_size=False)
- writer.error_check("message_start + %s + %s > message_end" % (v, array_item.nw_size()))
+ writer.error_check("%s + %s > (uintptr_t) (message_end - message_start)" % (v, array_item.nw_size()))
if want_extra_size:
if item.member and item.member.has_attr("chunk"):
@@ -308,11 +308,11 @@ def write_validate_array_item(writer, co
nelements = "%s__nbytes" %(item.prefix)
real_nelements = "%s__nelements" %(item.prefix)
if not parent_scope.variable_defined(real_nelements):
- parent_scope.variable_def("uint32_t", real_nelements)
+ parent_scope.variable_def("uint64_t", real_nelements)
else:
nelements = "%s__nelements" %(item.prefix)
if not parent_scope.variable_defined(nelements):
- parent_scope.variable_def("uint32_t", nelements)
+ parent_scope.variable_def("uint64_t", nelements)
if array.is_constant_length():
writer.assign(nelements, array.size)
@@ -407,10 +407,10 @@ def write_validate_array_item(writer, co
element_nw_size = element_item.nw_size()
element_mem_size = element_item.mem_size()
element_extra_size = element_item.extra_size()
- scope.variable_def("uint32_t", element_nw_size)
- scope.variable_def("uint32_t", element_mem_size)
+ scope.variable_def("uint64_t", element_nw_size)
+ scope.variable_def("uint64_t", element_mem_size)
if want_extra_size:
- scope.variable_def("uint32_t", element_extra_size)
+ scope.variable_def("uint64_t", element_extra_size)
if want_nw_size:
writer.assign(nw_size, 0)
@@ -543,7 +543,7 @@ def write_validate_container(writer, pre
sub_want_nw_size = want_nw_size and not m.is_fixed_nw_size()
sub_want_mem_size = m.is_extra_size() and want_mem_size
sub_want_extra_size = not m.is_extra_size() and m.contains_extra_size()
- defs = ["size_t"]
+ defs = ["uint64_t"]
name = prefix_m(prefix, m)
if sub_want_nw_size:
@@ -684,7 +684,7 @@ def read_array_len(writer, prefix, array
if dest.is_toplevel() and scope.variable_defined(nelements):
return nelements # Already there for toplevel, need not recalculate
element_type = array.element_type
- scope.variable_def("uint32_t", nelements)
+ scope.variable_def("uint64_t", nelements)
if array.is_constant_length():
writer.assign(nelements, array.size)
elif array.is_identifier_length():
@@ -1040,9 +1040,9 @@ def write_msg_parser(writer, message):
parent_scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
parent_scope.variable_def("uint8_t *", "start = message_start")
parent_scope.variable_def("uint8_t *", "data = NULL")
- parent_scope.variable_def("size_t", "nw_size")
+ parent_scope.variable_def("uint64_t", "nw_size")
if want_mem_size:
- parent_scope.variable_def("size_t", "mem_size")
+ parent_scope.variable_def("uint64_t", "mem_size")
if not message.has_attr("nocopy"):
parent_scope.variable_def("uint8_t *", "in", "end")
num_pointers = message.get_num_pointers()
@@ -1060,7 +1060,7 @@ def write_msg_parser(writer, message):
writer.newline()
writer.comment("Check if message fits in reported side").newline()
- with writer.block("if (start + nw_size > message_end)"):
+ with writer.block("if (nw_size > (uintptr_t) (message_end - start))"):
writer.statement("return NULL")
writer.newline().comment("Validated extents and calculated size").newline()
@@ -1071,7 +1071,7 @@ def write_msg_parser(writer, message):
writer.assign("*size", "message_end - message_start")
writer.assign("*free_message", "nofree")
else:
- writer.assign("data", "(uint8_t *)malloc(mem_size)")
+ writer.assign("data", "(uint8_t *)(mem_size > UINT32_MAX ? NULL : malloc(mem_size))")
writer.error_check("data == NULL")
writer.assign("end", "data + %s" % (msg_sizeof))
writer.assign("in", "start").newline()
>From 8e5cc958b915d6a2455d73dd47f0091d265a6b24 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fzig...@redhat.com>
Date: Fri, 19 Jun 2015 19:08:55 +0100
Subject: [PATCH spice-common 3/3] Avoid integer overflow computing image sizes
Use always 64, sizes can be 32x32.
TODO in case 32x32 can still be a problem :(
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
python_modules/demarshal.py | 14 ++++++--------
python_modules/marshal.py | 7 +++----
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
index 7e73985..8d3f5cb 100644
--- a/spice-common/python_modules/demarshal.py
+++ b/spice-common/python_modules/demarshal.py
@@ -346,13 +346,12 @@ def write_validate_array_item(writer, container, item, scope, parent_scope, star
rows = array.size[3]
width_v = write_read_primitive(writer, start, container, width, scope)
rows_v = write_read_primitive(writer, start, container, rows, scope)
- # TODO: Handle multiplication overflow
if bpp == 8:
- writer.assign(nelements, "%s * %s" % (width_v, rows_v))
+ writer.assign(nelements, "(uint64_t) %s * %s" % (width_v, rows_v))
elif bpp == 1:
- writer.assign(nelements, "((%s + 7) / 8 ) * %s" % (width_v, rows_v))
+ writer.assign(nelements, "(((uint64_t) %s + 7U) / 8U ) * %s" % (width_v, rows_v))
else:
- writer.assign(nelements, "((%s * %s + 7) / 8 ) * %s" % (bpp, width_v, rows_v))
+ writer.assign(nelements, "((%sU * (uint64_t) %s + 7U) / 8U ) * %s" % (bpp, width_v, rows_v))
elif array.is_bytes_length():
is_byte_size = True
v = write_read_primitive(writer, start, container, array.size[1], scope)
@@ -713,13 +712,12 @@ def read_array_len(writer, prefix, array, dest, scope, is_ptr):
rows = array.size[3]
width_v = dest.get_ref(width)
rows_v = dest.get_ref(rows)
- # TODO: Handle multiplication overflow
if bpp == 8:
- writer.assign(nelements, "%s * %s" % (width_v, rows_v))
+ writer.assign(nelements, "((uint64_t) %s * %s)" % (width_v, rows_v))
elif bpp == 1:
- writer.assign(nelements, "((%s + 7) / 8 ) * %s" % (width_v, rows_v))
+ writer.assign(nelements, "(((uint64_t) %s + 7U) / 8U ) * %s" % (width_v, rows_v))
else:
- writer.assign(nelements, "((%s * %s + 7) / 8 ) * %s" % (bpp, width_v, rows_v))
+ writer.assign(nelements, "((%sU * (uint64_t) %s + 7U) / 8U ) * %s" % (bpp, width_v, rows_v))
elif array.is_bytes_length():
writer.assign(nelements, dest.get_ref(array.size[2]))
else:
diff --git a/python_modules/marshal.py b/python_modules/marshal.py
index 402273c..fd3416a 100644
--- a/spice-common/python_modules/marshal.py
+++ b/spice-common/python_modules/marshal.py
@@ -172,13 +172,12 @@ def get_array_size(array, container_src):
rows = array.size[3]
width_v = container_src.get_ref(width)
rows_v = container_src.get_ref(rows)
- # TODO: Handle multiplication overflow
if bpp == 8:
- return "(unsigned) (%s * %s)" % (width_v, rows_v)
+ return "((uint64_t) %s * %s)" % (width_v, rows_v)
elif bpp == 1:
- return "(unsigned) (((%s + 7) / 8 ) * %s)" % (width_v, rows_v)
+ return "((((uint64_t) %s + 7U) / 8U ) * %s)" % (width_v, rows_v)
else:
- return "(unsigned) (((%s * %s + 7) / 8 ) * %s)" % (bpp, width_v, rows_v)
+ return "((((uint64_t) %s * %s + 7U) / 8U ) * %s)" % (bpp, width_v, rows_v)
elif array.is_bytes_length():
return container_src.get_ref(array.size[2])
else:
--
2.13.6
