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