summaryrefslogtreecommitdiff
path: root/usbredirparser
AgeCommit message (Collapse)AuthorFilesLines
2022-08-02Prepare for 0.13.0 releaseHEADusbredir-0.13.0masterVictor Toso1-1/+1
Signed-off-by: Victor Toso <victortoso@redhat.com>
2022-06-25usbredirparser: reset parser's fields on unserializeVictor Toso1-0/+15
This is a followup from previous commit and fixes the following leak. | 104 (24 direct, 80 indirect) bytes in 1 blocks are definitely lost in loss record 15 of 19 | at 0x484A464: calloc (vg_replace_malloc.c:1328) | by 0x485A238: usbredirparser_queue (usbredirparser.c:1235) | by 0x485A571: usbredirparser_init (usbredirparser.c:227) | by 0x40130B: get_usbredirparser (serializer.c:77) | by 0x401379: simple (serializer.c:95) | by 0x48FA3DD: ??? (in /usr/lib64/libglib-2.0.so.0.7200.2) | by 0x48FA144: ??? (in /usr/lib64/libglib-2.0.so.0.7200.2) | by 0x48FA8E1: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.7200.2) | by 0x48FA94C: g_test_run (in /usr/lib64/libglib-2.0.so.0.7200.2) | by 0x401161: main (serializer.c:112) | | LEAK SUMMARY: | definitely lost: 24 bytes in 1 blocks | indirectly lost: 80 bytes in 1 blocks | possibly lost: 0 bytes in 0 blocks | still reachable: 25,500 bytes in 17 blocks | suppressed: 0 bytes in 0 blocks | Reachable blocks (those to which a pointer was found) are not shown. | To see them, rerun with: --leak-check=full --show-leak-kinds=all Signed-off-by: Victor Toso <victortoso@redhat.com>
2022-06-25usbredirparser: Fix unserialize on pristine checkVictor Toso1-3/+1
As mentioned in the bug below, the user is trying to migrate QEMU and it is failing on the unserialization of usbredirparser at the target host. The user does not have USB attached to the VM at all. I've added a test that shows that serialization is currently broken. It fails at the 'pristine' check in usbredirparser_unserialize(). This check was added with e37d86c "Skip empty write buffers when unserializing parser" and restricted further with 186c4c7 "Avoid memory leak from ill-formatted serialization data" The issue here is that usbredirparser's initialization sets some fields and thus it isn't guaranteed to be pristine. The parser's basic data is: | write_buf_count ... : 1 | write_buf ........ : 0xbc03e0 | write_buf_total_size: 80 | data ............. : (nil) | header_read: ...... : 0 | type_header_read .. : 0 | data_read: ........ : 0 The current fix is to to ignore write_buf checks as, again, they are not guaranteed to be pristine. usbredirparser library should properly overwrite them when unserializing the data and if there were pending buffers, they should be freed. Related: https://bugzilla.redhat.com/show_bug.cgi?id=2096008 Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-12-30Check header length unserialising dataFrediano Ziglio1-6/+7
Avoid unwanted packets. The test for header length is moved outside the if. If the header is not complete the number will contain 0 bytes so a smaller number. This avoids potential excessive allocations if the header length is very high. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-11-04Prepare for 0.12.0 releaseusbredir-0.12.0Victor Toso1-2/+2
Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-11-04usbredirparser: introduce usbredirparser_get_bufferered_output_size()Victor Toso3-5/+39
The application does not have a way to know how much data is being queued by usbredirparser due data output being slower than input. This function is a simple way to get that value. Also, this function can be used in usbredirhost to allow dropping isoc packets before calling usbredirparser's functions. Related: https://gitlab.freedesktop.org/spice/usbredir/-/issues/19 Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-10-02Fix some issues detected by fuzzerFrediano Ziglio1-1/+7
If we fail to unserialize data we need to reset data to avoid invalid state. We can accept data only if we had data (data_len > 0), otherwise reset it. This also fixes https://gitlab.freedesktop.org/spice/usbredir/-/issues/21. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-09-29Limit packet size during deserializationMichael Hanselmann1-1/+7
Apply the packet size limit used when reading packets during deserialization. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-09-29Change data_len and data togetherFrediano Ziglio1-7/+9
The array pointed by data should be big to contain data_len bytes. Change one field close to the other to make easier to maintain such invariant in usbredirparser_unserialize. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-09-28Implement some internal checks to make sure usbredirparser_priv is correctFrediano Ziglio1-11/+82
Make sure structure keeps its integrity. This is useful used with the fuzzer. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-08-28Avoid memory leak from ill-formatted serialization dataMichael Hanselmann1-5/+13
At commit 060d7914ea the following `fuzzing/usbredirparserfuzz` input triggers a memory leak: ``` $ base64 -d <<'EOF' | gunzip -c > testcase H4sIAFDwJ2ECA2NgZIADBQWF/zD2HQgfAv4DgcJ/BTzgP0iRAlwRivL/ePX9 R1VAyBaSjcShB8nPTBA9/xn+g5UAANvH+dkSAQAA ``` The data type header segment is empty while there's (supposed) payload data in there. The internal buffers must be filled in the order of header, type header and then data, an invariant important for `usbredirparser_do_read` and violated by this input. With this change the input data is read the same way, but if the invariant would be violated the data read is just ignored. The parser check at the beginning of `usbredirparser_unserialize` is also improved and `write_buf_count` is no longer set explicitly. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-08-28usbredirparser: Use consistent indentationMichael Hanselmann1-13/+13
Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-08-23Update write buffer count during deserializationMichael Hanselmann1-0/+5
At commit 8490a7ac the following `fuzzing/usbredirparserfuzz` input causes an integer underflow on write_buf_count as it's not updated during deserialization: ``` $ base64 -d <<'EOF' | gunzip -c > testcase6250181968920576 H4sIAMChImECAzNkwA0YgZgTxPiPBGCSAkgK07b9YgPRLDA+EBvNYoPLuzDg A3Cj3/xHBTc5IPJgFQz/vwNJznSGBIYQIBtJPxN29n842xbhaohtaXA7GX4z fHsPFNVmYHiI4S8jBiKAADbfAEMOaEkCMEDArv6fBpX4gzMM/6OoxxHOWMMO YjYiIFleosvhsxcD/IYofY/dW0AD/xPy1nWc3kpDTUsEnYPD+UQEPzHuxB78 qWDwn0pgP8k6UvGDq0Alu7Foe0+0BYygEDBg+LwtAZhdAUAa2Kf/AwAA EOF ``` Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-08-23Skip empty write buffers when unserializing parserMichael Hanselmann1-4/+22
At commit 8490a7ac101d the following `fuzzing/usbredirparserfuzz` input causes the instantiation of empty write buffers: $ base64 -d <<'EOF' > testcase6474540506021888 QAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAYAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAG AAAAAN7/AAAACAA= EOF Empty write buffers trigger all kinds of issues, in part because they cause calls to write(2) with a zero length. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-08-13Move usbredirparser magic value to public headerMichael Hanselmann2-1/+2
Currently the fuzzer hardcodes the value of the USBREDIRPARSER_SERIALIZE_MAGIC constant. Move it to usbredirparser.h to allow for its reuse in the fuzzer. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-08-12Use "#pragma once" as include guardMichael Hanselmann5-21/+5
While "#pragma once" is not part of the C or C++ standards, it is a construct supported "by the vast majority of modern compilers"[1]. The previously used include guards starting with two underscores were also in violation of the relevant standards: identifiers starting with a double underscore ("__") are reserved for the implementation[2]. [1] https://en.cppreference.com/w/cpp/preprocessor/impl#.23pragma_once [2] https://eel.is/c++draft/lex.name#3.1 Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-08-09Prepare for 0.11.0 releaseusbredir-0.11.0Victor Toso1-3/+3
Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-08-08Avoid use-after-free in serializationMichael Hanselmann1-3/+5
Serializing parsers with large amounts of buffered write data (e.g. in case of a slow or blocked write destination) would cause "serialize_data" to reallocate the state buffer whose default size is 64kB (USBREDIRPARSER_SERIALIZE_BUF_SIZE). The pointer to the position for the write buffer count would then point to a location outside the buffer where the number of write buffers would be written as a 32-bit value. As of QEMU 5.2.0 the serializer is invoked for migrations. Serializations for migrations may happen regularily such as when using the COLO feature[1]. Serialization happens under QEMU's I/O lock. The guest can't control the state while the serialization is happening. The value written is the number of outstanding buffers which would be suceptible to timing and host system system load. The guest would have to continously groom the write buffers. A useful value needs to be allocated in the exact position freed during the buffer size increase, but before the buffer count is written. The author doesn't consider it realistic to exploit this use-after-free reliably. [1] https://wiki.qemu.org/Features/COLO Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-07-08Use enum value for parse errors in usbredirparser_do_readMichael Hanselmann1-6/+6
A bare `return -2` isn't as understandable as a `return usbredirparser_read_parse_error`. The latter is also the documented return value for parser errors. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-06-19Verify that rule separators are not emptyMichael Hanselmann1-0/+8
The `usbredirfilter_rules_to_string` function always dereferences its separator arguments, thus requiring at least one character. Parsing a rule string in `usbredirfilter_string_to_rules` can only work correctly when there's at least one separator character for both rules and tokens. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-06-18usbredirparser: free parser's data on destroyMichael Hanselmann1-0/+3
Ref: https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/25#note_962929 Signed-off-by: Michael Hanselmann <public@hansmi.ch> Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-06-18Release memory after handling packetMichael Hanselmann1-4/+17
These memory leaks were found with the usbredirparser fuzzer. The API transfers ownership of the data allocation only for the *_packet callbacks (also documented in "usbredirparser.h"). The hello packet handler already freed the payload while the others didn't. In addition destroying the parser structure must also destroy the read data buffer. With these changes it's finally possible to run in excess of 1.7 million fuzzing iterations without noticable memory leaks. LeakSanitizer no longer reports after a few iterations. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-06-09Provides a usbredirfilter_free functionFrediano Ziglio3-4/+20
Makes sure we release resources correctly if the system is using multiple free() implementations. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-06-07Use strtok style separator for usbredirfilter_string_to_rulesFrediano Ziglio2-10/+12
Follow strtok way to handle tokens using all set of separators and collapsing them. The function was already using strtok but had a bug preventing to use it correctly. Update documentation to explain the change. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-06-07Make filter string empty if no rules are passedFrediano Ziglio1-0/+1
Fix not terminated string. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-06-05Small glibc_strtok_r optimizationFrediano Ziglio1-3/+3
Avoids to scan part of the string twice for the last token. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-06-05Fix formatting warnings using some MingW versionFrediano Ziglio1-1/+3
Using PRId64 formatting (or other 64 bit formats) can lead to bug as GCC is detecting some invalid match between string format and parameters. This is due to wrong format used. This is very similar to 249f42aaaf182cfd4f4b9120b2468ec8721bd890 (cfr "Fix mingw build with __MINGW_PRINTF_FORMAT"), fixing the same pattern. Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-06-05filter: remove trailing rule_sep on serialization to stringVictor Toso1-1/+4
This is not included in the documentation so it can't be considered part of API. Note that the mirrored function usbredirfilter_string_to_rules() ignores trailing rule_sep and even empty rules. Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-06-04filter: update comment on upper limit of bcd versionVictor Toso1-1/+1
The actual limit in the code is 16bit size, see usbredirfilter_verify Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-06-04Use visibility to limit symbols exportedFrediano Ziglio3-3/+56
This mainly address MacOS issues using visibility. The .map files we use serve both at limiting symbols and provide symbol versioning. However to limit symbols using MacOS linker some different files need to be prepared and arguments to linker must be different. Using visibility is more supported by both Meson and multiple systems (Linux due to gcc or clang, MacOS due to clang, Windows). Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-06-03Fix typo in commentFrediano Ziglio1-1/+1
Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
2021-05-29Fix typos in header commentsMichael Hanselmann2-10/+10
Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-05-27Prepare for 0.10.0 releaseusbredir-0.10.0Victor Toso1-1/+1
Note that between 0.9.0 and 0.10.0, no changes were made in usbredirhost library, only in usbredirparser. Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-05-25Revert "Reduce allowed bulk transfer size to 64kB"Victor Toso1-2/+2
This reverts commit 2f25c527fd65046ecbb89d87fab4e51dfc537cea. Fixes: https://gitlab.freedesktop.org/spice/usbredir/-/issues/16 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1950258
2021-05-03build-sys: drop autotoolsVictor Toso2-30/+0
Meson build was added for 0.9.0 release where we deprecated autotools. Now that release is done, let's maintain only meson. Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-04-14Add local directory to include search path for mesonMichael Hanselmann1-2/+7
All includes of usbredir-related headers use the form `#include "usbredirparser.h"`, i.e. not relative to the root directory. As a consequence builds would fail to find files if they weren't installed in a global include directory: ``` ../usbredirhost/usbredirhost.h:25:10: fatal error: usbredirparser.h: No such file or directory ``` Adding "." to the include paths avoids this issue. Signed-off-by: Michael Hanselmann <public@hansmi.ch>
2021-04-02Prepare for 0.9.0 releaseusbredir-0.9.0Victor Toso1-1/+1
Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-03-24meson: Add missing usbredir{host parser} pc filesVictor Toso1-0/+7
Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-03-24build-sys: Ship meson on a tarball from autotoolsVictor Toso1-0/+3
The opposite is already true. You can build with autotools on a tarball made with meson dist. Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-03-24build-sys: Add meson supportVictor Toso2-0/+109
As an alternative to autotools. Simple time comparison between configure+build time: 11.569s ./autogen.sh && make 1.642s meson . _build && ninja -C _build Note that from cflags defined in autotools, only -fstack-protector was removed for the windows build, otherwise we get few of the following: > /usbredir/winbuild/../usbredirparser/usbredirfilter.c:274: undefined reference to `__stack_chk_fail' Signed-off-by: Victor Toso <victortoso@redhat.com>
2021-02-08Limit packet length to 65 kBMichael Hanselmann1-0/+12
The usbredir protocol allows for packet sizes up to 4 GiB (usb_redir_header.length is an uint32_t). In practice the conversion to a signed integer limits them to 2 GiB. With this change a limit of 65 kB is applied as no real packets should be that large. Signed-off-by: Michael Hanselmann <public@hansmi.ch> Acked-by: Frediano Ziglio <freddy77@gmail.com>
2021-02-08Reduce allowed bulk transfer size to 64kBMichael Hanselmann1-2/+2
QEMU bases the "bytes_per_transfer" value on "max_packet_size", a 16 bit value. Signed-off-by: Michael Hanselmann <public@hansmi.ch> Acked-by: Frediano Ziglio <freddy77@gmail.com>
2021-01-28Up-cast 16-bit integer before shifting by 16 bitsMichael Hanselmann1-3/+4
The usb_redir_bulk_packet_header.length_high field is of type uint16_t. Shifting it by 16 bits is undefined: "[…] a shift operand value which is […] or is greater than or equal to the total number of bits in this value results in undefined behavior" (explanation from Wikipedia). Also reported by UBSan: left shift of 65535 by 16 places cannot be represented in type 'int' With this change the length is always stored in an uint32_t. Signed-off-by: Michael Hanselmann <public@hansmi.ch> Acked-by: Frediano Ziglio <freddy77@gmail.com>
2017-07-28Avoid format truncation warnings on newer gccJonathon Jongsma1-1/+2
Newer versions of gcc (e.g. 7.1.1 in fedora 26) print a warning about format truncation even when using snprintf: CC usbredirparser.lo ../../usbredirparser/usbredirparser.c: In function ‘usbredirparser_do_read’: ../../usbredirparser/usbredirparser.c:270:33: error: ‘%s’ directive output may be truncated writing up to 287 bytes into a region of size 64 [-Werror=format-truncation=] snprintf(buf, sizeof(buf), "%s", hello->version); ^~ ../../usbredirparser/usbredirparser.c:270:5: note: ‘snprintf’ output between 1 and 288 bytes into a destination of size 64 snprintf(buf, sizeof(buf), "%s", hello->version); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Because type_header is a 288-byte array that is cast to a 'struct usb_redir_hello_header' and passed to the function, gcc apparently believes that hello->version might be up to 288 bytes and warns about format truncation. To avoid this warning, simply use strncpy (and ensure that the last byte is NULL).
2016-08-03usbredirparser: prevent endless recursion if interface_count == 0Alon Levy1-1/+1
On fedora 24 this function is tail optimized, resulting in a busy wait. This happens to me with virt-manager running a win7 vm usbredir-0.7.1-2.fc24.x86_64
2015-12-18Remove trailing whitespaceLukas Venhoda2-8/+8
2015-11-16Fix various typos in usbredirfilter.h commentsChristophe Fergeau1-4/+4
2015-10-29Prepare for 0.7.1 releaseusbredir-0.7.1Victor Toso1-1/+1
Signed-off-by: Victor Toso <victortoso@redhat.com>
2015-10-23Allow missing capabilities from source hostDr. David Alan Gilbert1-4/+18
When loading a USB redirection stream during a qemu migration, the source QEMU might be earlier and be missing a bunch of capabilities that are now available in a more modern version; allow this migration to work as long as the source isn't claiming any capabilities that we don't have. (We should be a bit more careful about this in future in qemu; we could tie any new capabilities we ask for to machine types). Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
2015-10-16usbredirfilter_check: force check a device if all its interfaces are skippedUri Lublin1-3/+18
See usbredirfilter.h for when interfaces are skipped. Force filter check on such a device by calling recursively with a flag that forbids skipping (usbredirfilter_fl_dont_skip_non_boot_hid) Related rhbz#1179210
generated by cgit v1.2.3 (git 2.39.1) at 2024-06-05 00:53:31 +0000