On 10/07/18 07:31, Frediano Ziglio wrote: >> >> On Tue, Jul 10, 2018 at 04:51:35AM -0400, Frediano Ziglio wrote: >>>> >>>> On Tue, Jul 10, 2018 at 07:21:50AM +0100, Frediano Ziglio wrote: >>>>> test-overflow was doing a specific test on demarshalling code. >>>>> Joining the 2 tests also allows to remove the dependency from the main >>>>> protocol allowing to run the test independently from generation >>>>> setting. >>>> >>>> >>>> >>>>> This is useful with Meson allowing to not generate all code. >>>> >>>> Fwiw, I don't understand the rationale here. >>>> >>> >>> The "if spice_common_generate_code == 'all'" code in meson.build, >>> basically the test was only possible if the code compiled everything >> >> But why is this a problem? :) Because we only need to generate the code >> for tests/*.proto, and generating more is a waste of resources? Or is >> this a problem for other reasons? >> >> Christophe >> > > Oh... I remember Eduardo wanting to not compile with spice_common_generate_code == 'all' > by default so to rnu the tests you would need to change the defaults. > Just a small optimization when building either spice server or spice-gtk, we only generate the specific marshallers/demarshallers for that given case. If building spice-common itself it will generate code for both. > By the way, I think this is a separate test as initially was in a security > series so detached from main development, I don't think we really need a > specific test to check a specific demarshalling issue. > >>> >>> Frediano >>> >>>> Christophe >>>> >>>>> >>>>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> >>>>> --- >>>>> tests/Makefile.am | 17 ++----- >>>>> tests/meson.build | 6 +-- >>>>> tests/test-marshallers.c | 81 +++++++++++++++++++++++++++++++-- >>>>> tests/test-marshallers.h | 5 +++ >>>>> tests/test-marshallers.proto | 5 +++ >>>>> tests/test-overflow.c | 87 ------------------------------------ >>>>> 6 files changed, 92 insertions(+), 109 deletions(-) >>>>> delete mode 100644 tests/test-overflow.c >>>>> >>>>> diff --git a/tests/Makefile.am b/tests/Makefile.am >>>>> index 1021954..926ac99 100644 >>>>> --- a/tests/Makefile.am >>>>> +++ b/tests/Makefile.am >>>>> @@ -70,6 +70,7 @@ TEST_MARSHALLERS = \ >>>>> generated_test_marshallers.c \ >>>>> generated_test_marshallers.h \ >>>>> generated_test_demarshallers.c \ >>>>> + generated_test_enums.h \ >>>>> $(NULL) >>>>> >>>>> BUILT_SOURCES = $(TEST_MARSHALLERS) >>>>> @@ -92,6 +93,8 @@ generated_test_marshallers.h: >>>>> $(srcdir)/test-marshallers.proto $(MARSHALLERS_DEP >>>>> $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py >>>>> --generate-marshallers --server --include test-marshallers.h -H $< $@ >>>>> >/dev/null >>>>> generated_test_demarshallers.c: $(srcdir)/test-marshallers.proto >>>>> $(MARSHALLERS_DEPS) >>>>> $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py >>>>> --generate-demarshallers --client --include test-marshallers.h $< $@ >>>>> >/dev/null >>>>> +generated_test_enums.h: $(srcdir)/test-marshallers.proto >>>>> $(MARSHALLERS_DEPS) >>>>> + $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py -e $< $@ >>>>>> /dev/null >>>>> >>>>> EXTRA_DIST = \ >>>>> $(TEST_MARSHALLERS) \ >>>>> @@ -99,18 +102,4 @@ 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 >>>>> diff --git a/tests/meson.build b/tests/meson.build >>>>> index 94c72c6..e53fd64 100644 >>>>> --- a/tests/meson.build >>>>> +++ b/tests/meson.build >>>>> @@ -4,11 +4,6 @@ >>>>> tests = ['test-logging', 'test-region'] >>>>> tests_deps = [spice_common_dep] >>>>> >>>>> -if spice_common_generate_code == 'all' >>>>> - tests += ['test-overflow'] >>>>> - tests_deps += [spice_common_client_dep, spice_common_server_dep] >>>>> -endif >>>>> - >>>>> foreach t : tests >>>>> name = t.underscorify() >>>>> exe = executable(name, '@0@.c'.format(t), >>>>> @@ -28,6 +23,7 @@ targets = [ >>>>> ['test_marshallers', test_proto, 'generated_test_marshallers.c', >>>>> ['--generate-marshallers', '--server', '--include', >>>>> 'test-marshallers.h', '@INPUT@', '@OUTPUT@']], >>>>> ['test_marshallers_h', test_proto, 'generated_test_marshallers.h', >>>>> ['--generate-marshallers', '--server', '--include', >>>>> 'test-marshallers.h', '-H', '@INPUT@', '@OUTPUT@']], >>>>> ['test_demarshallers', test_proto, >>>>> 'generated_test_demarshallers.c', >>>>> ['--generate-demarshallers', '--client', '--include', >>>>> 'test-marshallers.h', '@INPUT@', '@OUTPUT@']], >>>>> + ['test_enums_h', test_proto, 'generated_test_enums.h', ['-e', >>>>> '@INPUT@', '@OUTPUT@']], >>>>> ] >>>>> >>>>> foreach t : targets >>>>> diff --git a/tests/test-marshallers.c b/tests/test-marshallers.c >>>>> index 3bd98e8..ad45e36 100644 >>>>> --- a/tests/test-marshallers.c >>>>> +++ b/tests/test-marshallers.c >>>>> @@ -1,13 +1,85 @@ >>>>> +/* >>>>> + Copyright (C) 2015-2018 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 <glib.h> >>>>> #include <string.h> >>>>> >>>>> -#include "common/marshaller.h" >>>>> +#include <common/marshaller.h> >>>>> +#include <common/client_demarshallers.h> >>>>> +#include "generated_test_enums.h" >>>>> #include "generated_test_marshallers.h" >>>>> >>>>> #ifndef g_assert_true >>>>> #define g_assert_true g_assert >>>>> #endif >>>>> >>>>> +#define NUM_CHANNELS 3u >>>>> + >>>>> +void test_overflow(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; >>>>> + >>>>> + msg = (SpiceMsgChannels *) malloc(sizeof(SpiceMsgChannels) + >>>>> + NUM_CHANNELS * sizeof(uint16_t)); >>>>> + g_assert_nonnull(msg); >>>>> + >>>>> + // build a message and marshal it >>>>> + msg->num_of_channels = NUM_CHANNELS; >>>>> + for (n = 0; n < NUM_CHANNELS; ++n) { >>>>> + msg->channels[n] = n + 1; >>>>> + } >>>>> + spice_marshall_msg_main_channels_list(m, msg); >>>>> + >>>>> + // get linear data >>>>> + data = spice_marshaller_linearize(m, 0, &len, &to_free); >>>>> + g_assert_nonnull(data); >>>>> + >>>>> + printf("output len %lu\n", (unsigned long) len); >>>>> + >>>>> + // hack: setting the number of channels in the marshalled message >>>>> to a >>>>> + // value that will cause overflow while parsing the message to >>>>> make >>>>> sure >>>>> + // that the parser can handle this situation >>>>> + *((uint32_t *) data) = 0x80000002u; >>>>> + >>>>> + // extract the message >>>>> + func = spice_get_server_channel_parser(SPICE_CHANNEL_MAIN, >>>>> &max_message_type); >>>>> + g_assert_nonnull(func); >>>>> + out = func(data, data+len, SPICE_MSG_MAIN_CHANNELS_LIST, 0, &len, >>>>> &free_output); >>>>> + g_assert_null(out); >>>>> + >>>>> + // cleanup >>>>> + if (to_free) { >>>>> + free(data); >>>>> + } >>>>> + if (out) { >>>>> + free_output(out); >>>>> + } >>>>> + free(msg); >>>>> +} >>>>> + >>>>> static uint8_t expected_data[] = { 123, /* dummy byte */ >>>>> 0x02, 0x00, 0x00, 0x00, /* >>>>> data_size */ >>>>> 0x09, 0x00, 0x00, 0x00, /* data >>>>> offset >>>>> */ >>>>> @@ -47,8 +119,9 @@ int main(int argc G_GNUC_UNUSED, char **argv >>>>> G_GNUC_UNUSED) >>>>> g_free(msg); >>>>> >>>>> // test demarshaller >>>>> - msg = (SpiceMsgMainShortDataSubMarshall *) spice_parse_msg(data, >>>>> data >>>>> + len, 1, 1, 0, >>>>> - >>>>> &msg_len, >>>>> &free_message); >>>>> + msg = (SpiceMsgMainShortDataSubMarshall *) >>>>> + spice_parse_msg(data, data + len, SPICE_CHANNEL_MAIN, >>>>> SPICE_MSG_MAIN_SHORTDATASUBMARSHALL, >>>>> + 0, &msg_len, &free_message); >>>>> >>>>> g_assert_nonnull(msg); >>>>> g_assert_cmpint(msg->dummy_byte, ==, 123); >>>>> @@ -75,6 +148,8 @@ int main(int argc G_GNUC_UNUSED, char **argv >>>>> G_GNUC_UNUSED) >>>>> free(data); >>>>> } >>>>> >>>>> + test_overflow(marshaller); >>>>> + >>>>> spice_marshaller_destroy(marshaller); >>>>> >>>>> return 0; >>>>> diff --git a/tests/test-marshallers.h b/tests/test-marshallers.h >>>>> index 46263d7..99877c0 100644 >>>>> --- a/tests/test-marshallers.h >>>>> +++ b/tests/test-marshallers.h >>>>> @@ -16,5 +16,10 @@ typedef struct { >>>>> uint16_t n; >>>>> } SpiceMsgMainZeroes; >>>>> >>>>> +typedef struct SpiceMsgChannels { >>>>> + uint32_t num_of_channels; >>>>> + uint16_t channels[0]; >>>>> +} SpiceMsgChannels; >>>>> + >>>>> #endif /* _H_TEST_MARSHALLERS */ >>>>> >>>>> diff --git a/tests/test-marshallers.proto >>>>> b/tests/test-marshallers.proto >>>>> index 08d3e01..c75134e 100644 >>>>> --- a/tests/test-marshallers.proto >>>>> +++ b/tests/test-marshallers.proto >>>>> @@ -14,6 +14,11 @@ channel TestChannel { >>>>> uint16 n; >>>>> uint32 res2 @zero; >>>>> } Zeroes; >>>>> + >>>>> + message { >>>>> + uint32 num_of_channels; >>>>> + uint16 channels[num_of_channels] @end; >>>>> + } @ctype(SpiceMsgChannels) channels_list; >>>>> }; >>>>> >>>>> protocol Spice { >>>>> diff --git a/tests/test-overflow.c b/tests/test-overflow.c >>>>> deleted file mode 100644 >>>>> index c972a25..0000000 >>>>> --- a/tests/test-overflow.c >>>>> +++ /dev/null >>>>> @@ -1,87 +0,0 @@ >>>>> -/* >>>>> - 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 <spice/enums.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: setting the number of channels in the marshalled message >>>>> to a >>>>> - // value that will cause overflow while parsing the message to >>>>> make >>>>> sure >>>>> - // that the parser can handle this situation >>>>> - *((uint32_t *) data) = 0x80000002u; >>>>> - >>>>> - // extract the message >>>>> - func = spice_get_server_channel_parser(SPICE_CHANNEL_MAIN, >>>>> &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); >>>>> - spice_marshaller_destroy(m); >>>>> - >>>>> - return 0; >>>>> -} >>>>> - > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel > -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etrunko@xxxxxxxxxx _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel