Re: [PATCH spice-common] tests: Join test-overflow and test-marshallers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]