hw/usb/redirect.c: usbredir_buffered_bulk_packet() may leak memory (or worse)

This is uncertain report about potential issues. I felt this is better to report (and you may close) rather than forget. I suggest proper code review around these potential issues by people familiar with the code. Edit: I also suggest addition of source code comments that would have eliminated these concerns.

Skimming the code for a CVE-2021-3682 fix backport, I noticed that the combination of commits e8ce12d9 and 5e796671 results in presence of a code path that doesn't appear to free "data" at all.

Specifically, if bufp_alloc() reaches its:

            free(free_on_destroy);
            return -1;

when inside other than the last iteration of this loop in usbredir_buffered_bulk_packet():

    free_on_destroy = NULL;
    for (i = 0; i < data_len; i += len) {
        int r;
        if (len >= (data_len - i)) {
            len = data_len - i;
            status = buffered_bulk_packet->status;
            free_on_destroy = data;
        }
        /* bufp_alloc also adds the packet to the ep queue */
        r = bufp_alloc(dev, data + i, len, status, ep, free_on_destroy);
        if (r) {
            break;
        }
    }

then it'd do a "free(NULL);" (a valid no-op) and the loop would end. If it's also the first iteration of the loop, then "data" would not have been added to any queue element, and so usbredir_buffered_bulk_packet() would return without freeing it, which looks inconsistent with its other error returns.

I wasn't able to quickly see where that function is called from (if at all) since it's only assigned to a function pointer and that name is not seen in the same source tree at all (at least verbatim):

$ grep -r usbredir_buffered_bulk_packet .
./hw/usb/redirect.c:static void usbredir_buffered_bulk_packet(void *priv, uint64_t id,
./hw/usb/redirect.c:    dev->parser->buffered_bulk_packet_func = usbredir_buffered_bulk_packet;
./hw/usb/redirect.c:static void usbredir_buffered_bulk_packet(void *priv, uint64_t id,
$ grep -r buffered_bulk_packet_func .
./hw/usb/redirect.c:    dev->parser->buffered_bulk_packet_func = usbredir_buffered_bulk_packet;

So I don't know what the callers' actual expectations are.

Potentially worse:

If it's not the first iteration, then "data" would be in some queue element(s) already, but this begs the question of how that is freed later without risky double-free (between different queue elements later, as well as between usbredir_buffered_bulk_packet() possibly having freed data on the last iteration when some queue elements may already store that pointer). I did not look into this at all.