Re: [PATCH spice-server v2 7/7] test-smardcard: Improve test coverage

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

 



Hi,

On Tue, Oct 08, 2019 at 06:39:24PM +0100, Frediano Ziglio wrote:
> Using coverage utility exercise more code paths:
> - message from channel with wrong type;
> - remove message from channel with already removed reader;
> - init message from channel (ignored);
> - data from devices, ADPU;
> - error from device;
> - messages split in different ways;
> - invalid reader_id values.
> 
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>

Acked-by: Victor Toso <victortoso@xxxxxxxxxx>

Nothing important to say, if we need to improve, we can wait till
bugs are hit first. Happy to have this tests, many thanks!

> ---
>  server/tests/test-smartcard.c | 207 ++++++++++++++++++++++++++++++----
>  server/tests/vmc-emu.c        |   3 +
>  server/tests/vmc-emu.h        |   3 +
>  3 files changed, 193 insertions(+), 20 deletions(-)
> 
> diff --git a/server/tests/test-smartcard.c b/server/tests/test-smartcard.c
> index aac2e7941..a3a7a1597 100644
> --- a/server/tests/test-smartcard.c
> +++ b/server/tests/test-smartcard.c
> @@ -41,6 +41,15 @@ static SpiceCoreInterface *core;
>  static Test *test;
>  static VmcEmu *vmc;
>  typedef int TestFixture;
> +static int client_socket = -1;
> +// buffer when data from channel are stored
> +static SpiceBuffer channel_buf;
> +// expected buffer in channel
> +static SpiceBuffer channel_expected;
> +// expected buffer in device
> +static SpiceBuffer device_expected;
> +
> +static void next_test(void);
>  
>  static void test_smartcard_setup(TestFixture *fixture, gconstpointer user_data)
>  {
> @@ -101,7 +110,7 @@ static void send_ack_sync(int socket, uint32_t generation)
>      g_assert_cmpint(socket_write(socket, &msg.type, 10), ==, 10);
>  }
>  
> -static void send_data(int socket, uint32_t type)
> +static void send_data(int socket, uint32_t type, uint32_t reader_id)
>  {
>      struct {
>          uint16_t dummy;
> @@ -114,24 +123,183 @@ static void send_data(int socket, uint32_t type)
>      msg.type = GUINT16_TO_LE(SPICE_MSGC_SMARTCARD_DATA);
>      msg.len = GUINT32_TO_LE(sizeof(VSCMsgHeader)+6);
>      msg.vheader.type = GUINT32_TO_LE(type);
> -    msg.vheader.reader_id = 0;
> +    msg.vheader.reader_id = GUINT32_TO_LE(reader_id);
>      msg.vheader.length = GUINT32_TO_LE(6);
>      strcpy(msg.data, "hello");
>  
>      g_assert_cmpint(socket_write(socket, &msg.type, sizeof(msg)-4), ==, sizeof(msg)-4);
>  }
>  
> -static void check_data(void *opaque)
> +static void check_data(VmcEmu *vmc)
> +{
> +    g_assert_cmpint(device_expected.offset, !=, 0);
> +    if (vmc->write_pos < device_expected.offset) {
> +        return;
> +    }
> +    g_assert_cmpint(vmc->write_pos, ==, device_expected.offset);
> +    g_assert_true(memcmp(vmc->write_buf, device_expected.buffer, device_expected.offset) == 0);
> +
> +    next_test();
> +}
> +
> +static void data_from_channel(int fd, int event, void *opaque)
> +{
> +    uint8_t buf[128];
> +    ssize_t ret = socket_read(fd, buf, sizeof(buf));
> +    if (ret <= 0) {
> +        return;
> +    }
> +    spice_buffer_append(&channel_buf, buf, ret);
> +
> +    g_assert_cmpint(channel_expected.offset, !=, 0);
> +    if (channel_buf.offset < channel_expected.offset) {
> +        return;
> +    }
> +    g_assert_true(memcmp(channel_buf.buffer, channel_expected.buffer, channel_expected.offset) == 0);
> +    spice_buffer_remove(&channel_buf, channel_expected.offset);
> +
> +    next_test();
> +}
> +
> +static void next_test(void)
>  {
> -    static const char expected_buf[] =
> -        // forwarded ReaderAdd message, note that payload is stripped
> -        "\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00"
> -        // forwarded APDU message
> -        "\x00\x00\x00\x07\x00\x00\x00\x00\x00\x00\x00\x06\x68\x65\x6c\x6c\x6f\x00";
> -    const size_t expected_buf_len = sizeof(expected_buf) - 1;
> -    g_assert_cmpint(vmc->write_pos, ==, expected_buf_len);
> -    g_assert_true(memcmp(vmc->write_buf, expected_buf, expected_buf_len) == 0);
> -    basic_event_loop_quit();
> +    static int test_num;
> +
> +    test_num++;
> +    printf("Executing subtest %d\n", test_num);
> +
> +    spice_buffer_reset(&channel_expected);
> +    spice_buffer_reset(&device_expected);
> +
> +    switch (test_num) {
> +    // First test, send some message to channel expecting a reply
> +    // for each message we are sending
> +    case 1: {
> +        static const char expected_buf[] =
> +            // forwarded ReaderAdd message, note that payload is stripped
> +            "\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00"
> +            // forwarded APDU message
> +            "\x00\x00\x00\x07\x00\x00\x00\x00\x00\x00\x00\x06\x68\x65\x6c\x6c\x6f\x00"
> +            "\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00";
> +        spice_buffer_append(&device_expected, expected_buf, sizeof(expected_buf) - 1);
> +
> +        send_data(client_socket, VSC_ReaderAdd, 0);
> +        send_data(client_socket, VSC_APDU, 0);
> +        send_data(client_socket, VSC_ReaderRemove, 0);
> +        } break;
> +    // Second test, send an init and remove a reader that is not present,
> +    // we expect an error for the removal (the Init is ignored)
> +    case 2: {
> +        static const char expected_buf[] =
> +            // forwarded Error message
> +            "\x65\x00\x10\x00\x00\x00"
> +            "\x02\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x01\x00\x00\x00";
> +        spice_buffer_append(&channel_expected, expected_buf, sizeof(expected_buf) - 1);
> +
> +        // Init message, ignored
> +        send_data(client_socket, VSC_Init, 0);
> +        // remove again, this will trigger an error
> +        send_data(client_socket, VSC_ReaderRemove, 0);
> +        } break;
> +    // Third test, APDU messages from device are forwarded to the channel.
> +    // We split the header and payload of the first message to check device code can handle it.
> +    // The second message is send inside a block with the end of the first to trigger
> +    // an hard path in the device code
> +    case 3: {
> +        static const char expected_buf[] =
> +            // forwarded APDU message
> +            "\x65\x00\x12\x00\x00\x00"
> +            "\x07\x00\x00\x00\x00\x00\x00\x00\x06\x00\x00\x00" "foobaz"
> +            "\x65\x00\x12\x00\x00\x00"
> +            "\x07\x00\x00\x00\x00\x00\x00\x00\x06\x00\x00\x00" "foobar";
> +        spice_buffer_append(&channel_expected, expected_buf, sizeof(expected_buf) - 1);
> +
> +        vmc_emu_reset(vmc);
> +        // data from device
> +        uint8_t *p = vmc->message;
> +
> +        // add VSC_APDU message
> +        memcpy(p, "\x00\x00\x00\x07\x00\x00\x00\x00\x00\x00\x00\x06" "foobaz", 18);
> +        p += 18;
> +        memcpy(p, "\x00\x00\x00\x07\x00\x00\x00\x00\x00\x00\x00\x06" "foobar", 18);
> +        p += 18;
> +        vmc_emu_add_read_till(vmc, vmc->message + 8);
> +        vmc_emu_add_read_till(vmc, vmc->message + 14);
> +        vmc_emu_add_read_till(vmc, p);
> +
> +        spice_server_char_device_wakeup(&vmc->instance);
> +        } break;
> +    // Fourth test, we should get back an error if client tried to remove
> +    // a not existing reader
> +    case 4: {
> +        static const char expected_buf[] =
> +            // forwarded Error message
> +            "\x65\x00\x10\x00\x00\x00"
> +            "\x02\x00\x00\x00\x05\x00\x00\x00\x04\x00\x00\x00\x01\x00\x00\x00";
> +        spice_buffer_append(&channel_expected, expected_buf, sizeof(expected_buf) - 1);
> +
> +        // remove invalid, this will trigger an error
> +        send_data(client_socket, VSC_ReaderRemove, 5);
> +        } break;
> +    // Fifth test, similar to previous but using an huge reader_id field to trigger
> +    // possible buffer overflow
> +    case 5: {
> +        static const char expected_buf[] =
> +            // forwarded Error message
> +            "\x65\x00\x10\x00\x00\x00"
> +            "\x02\x00\x00\x00\x05\x01\x00\x00\x04\x00\x00\x00\x01\x00\x00\x00";
> +        spice_buffer_append(&channel_expected, expected_buf, sizeof(expected_buf) - 1);
> +
> +        // remove invalid and huge, this will trigger an error, should not crash
> +        send_data(client_socket, VSC_ReaderRemove, 261);
> +        } break;
> +    // Sixth test, send an invalid message from client, a log is triggered
> +    // but channel continues to work
> +    case 6: {
> +        static const char expected_buf[] =
> +            // forwarded APDU message
> +            "\x00\x00\x00\x07\x00\x00\x00\x00\x00\x00\x00\x06\x68\x65\x6c\x6c\x6f\x00";
> +        spice_buffer_append(&device_expected, expected_buf, sizeof(expected_buf) - 1);
> +
> +        g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING,
> +                              "*ERROR: unexpected message on smartcard channel*");
> +
> +        // invalid message type, should log a warning
> +        send_data(client_socket, 0xabcd, 0);
> +        // APDU just to get an event
> +        send_data(client_socket, VSC_APDU, 0);
> +        } break;
> +    // Seventh test, an Error message from device are forwarded to the channel.
> +    // Note that the header is in big endian order while the error from device
> +    // is in little endian order. This seems weird but it's correct with the
> +    // current libcacard implementation which just send error as host order
> +    case 7: {
> +        g_test_assert_expected_messages();
> +
> +        static const char expected_buf[] =
> +            // forwarded Error message
> +            "\x65\x00\x10\x00\x00\x00"
> +            "\x02\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x0a\x0b\x0c\x0d";
> +        spice_buffer_append(&channel_expected, expected_buf, sizeof(expected_buf) - 1);
> +
> +        vmc_emu_reset(vmc);
> +        // data from device
> +        uint8_t *p = vmc->message;
> +
> +        // add Error message
> +        memcpy(p, "\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x04\x0a\x0b\x0c\x0d", 16);
> +        p += 16;
> +        vmc_emu_add_read_till(vmc, p);
> +
> +        spice_server_char_device_wakeup(&vmc->instance);
> +        } break;
> +    case 8:
> +        g_test_assert_expected_messages();
> +        basic_event_loop_quit();
> +        break;
> +    default:
> +        abort();
> +    }
>  }
>  
>  static void test_smartcard(TestFixture *fixture, gconstpointer user_data)
> @@ -144,6 +312,7 @@ static void test_smartcard(TestFixture *fixture, gconstpointer user_data)
>      // add VSC_Init message
>      memcpy(p, "\x00\x00\x00\x01\x0a\x0b\x0c\x0d\x00\x00\x00\x00", 12);
>      p += 12;
> +    vmc_emu_add_read_till(vmc, vmc->message + 2); // check header is decoded correctly when split
>      vmc_emu_add_read_till(vmc, p);
>  
>      // find Smartcard channel to connect to
> @@ -170,7 +339,6 @@ static void test_smartcard(TestFixture *fixture, gconstpointer user_data)
>      red_client_set_main(client, mcc);
>  
>      // create our testing RedChannelClient
> -    int client_socket;
>      red_channel_connect(channel, client, create_dummy_stream(server, &client_socket),
>                          FALSE, &caps);
>      red_channel_capabilities_reset(&caps);
> @@ -180,21 +348,20 @@ static void test_smartcard(TestFixture *fixture, gconstpointer user_data)
>  
>      // push data into channel
>      send_ack_sync(client_socket, 1);
> -    send_data(client_socket, VSC_ReaderAdd);
> -    send_data(client_socket, VSC_APDU);
>  
> -    // check data are processed after a short time
> -    SpiceTimer *watch_timer;
> -    watch_timer = core->timer_add(check_data, core);
> -    core->timer_start(watch_timer, 100);
> +    // check data are processed
> +    SpiceWatch *watch = core->watch_add(client_socket, SPICE_WATCH_EVENT_READ,
> +                                        data_from_channel, NULL);
> +    vmc->data_written_cb = check_data;
>  
>      // start all test
>      alarm(10);
> +    next_test();
>      basic_event_loop_mainloop();
>      alarm(0);
>      // cleanup
> -    core->timer_remove(watch_timer);
> +    core->watch_remove(watch);
>      red_client_destroy(client);
>      g_object_unref(main_channel);
>      g_object_unref(channel);
> diff --git a/server/tests/vmc-emu.c b/server/tests/vmc-emu.c
> index ddac5269c..9ef05061a 100644
> --- a/server/tests/vmc-emu.c
> +++ b/server/tests/vmc-emu.c
> @@ -31,6 +31,9 @@ static int vmc_write(SpiceCharDeviceInstance *sin,
>      unsigned copy = MIN(sizeof(vmc->write_buf) - vmc->write_pos, len);
>      memcpy(vmc->write_buf+vmc->write_pos, buf, copy);
>      vmc->write_pos += copy;
> +    if (copy && vmc->data_written_cb) {
> +        vmc->data_written_cb(vmc);
> +    }
>      return len;
>  }
>  
> diff --git a/server/tests/vmc-emu.h b/server/tests/vmc-emu.h
> index 6fcea69d2..a6b77f585 100644
> --- a/server/tests/vmc-emu.h
> +++ b/server/tests/vmc-emu.h
> @@ -40,6 +40,9 @@ struct VmcEmu {
>  
>      unsigned write_pos;
>      uint8_t write_buf[2048];
> +
> +    // this callback will be called when new data arrive to the device
> +    void (*data_written_cb)(VmcEmu *vmc);
>  };
>  
>  VmcEmu *vmc_emu_new(const char *subtype, const char *portname);
> -- 
> 2.21.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

_______________________________________________
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]