Merge branch 'avb-test-suite' into 'master'

module-avb: fix heap corruption in server_destroy_descriptors

See merge request pipewire/pipewire!2778
This commit is contained in:
Christian Fredrik Kalager Schaller 2026-04-08 19:26:28 +00:00
commit 3d0545bf3f
14 changed files with 4784 additions and 31 deletions

143
test/avb-bugs.md Normal file
View file

@ -0,0 +1,143 @@
# AVB Module Bugs Found via Test Suite
The following bugs were discovered by building a software test harness
for the AVB protocol stack. All have been fixed in the accompanying
patch series.
## 1. Heap corruption in server_destroy_descriptors
**File:** `src/modules/module-avb/internal.h`
**Commit:** `69c721006`
`server_destroy_descriptors()` called `free(d->ptr)` followed by
`free(d)`, but `d->ptr` points into the same allocation as `d`
(set via `SPA_PTROFF(d, sizeof(struct descriptor), void)` in
`server_add_descriptor()`). This is a double-free / heap corruption
that could cause crashes or memory corruption when tearing down an
AVB server.
**Fix:** Remove the erroneous `free(d->ptr)` call.
## 2. NULL pointer dereference in MSRP notify dispatch
**File:** `src/modules/module-avb/msrp.c`, `src/modules/module-avb/mvrp.c`
**Commit:** `b056e9f85`
`msrp_notify()` unconditionally calls `dispatch[a->attr.type].notify()`
but the dispatch table entry for `AVB_MSRP_ATTRIBUTE_TYPE_TALKER_FAILED`
has `notify = NULL`. If a talker-failed attribute receives a registrar
state change (e.g., `RX_NEW` triggers `NOTIFY_NEW`), this crashes with
a NULL pointer dereference. The same unguarded pattern exists in
`mvrp_notify()`.
**Fix:** Add `if (dispatch[a->attr.type].notify)` NULL check before
calling, matching the defensive pattern already used in the encode path.
## 3. MRP NEW messages never transmitted
**File:** `src/modules/module-avb/mrp.h`, `src/modules/module-avb/mrp.c`,
`src/modules/module-avb/msrp.c`, `src/modules/module-avb/mvrp.c`
**Commit:** `bc2c41daa`
`AVB_MRP_SEND_NEW` was defined as `0`. The MSRP and MVRP event handlers
skip attributes with `if (!a->attr.mrp->pending_send)`, treating `0` as
"no pending send". Since the MRP state machine sets `pending_send` to
`AVB_MRP_SEND_NEW` (0) when an attribute in state VN or AN receives a
TX event, NEW messages were silently dropped instead of being
transmitted. This violates IEEE 802.1Q which requires NEW messages to
be sent when an attribute is first declared.
In practice, the attribute would cycle through VN -> AN -> AA over
successive TX events, eventually sending a JOINMT instead of the
initial NEW. The protocol still functioned because JOINMT also
registers the attribute, but the initial declaration was lost.
**Fix:** Shift all `AVB_MRP_SEND_*` values to start at 1, so that 0
unambiguously means "no send pending". Update MSRP and MVRP encoders
to subtract 1 when encoding to the IEEE 802.1Q wire format.
## 4. ACMP error responses sent with wrong message type
**File:** `src/modules/module-avb/acmp.c`
**Commit:** `9f4147104`
In `handle_connect_tx_command()` and `handle_disconnect_tx_command()`,
`AVB_PACKET_ACMP_SET_MESSAGE_TYPE()` is called after the `goto done`
jump target. When `find_stream()` fails (returns NULL), the code jumps
to `done:` without setting the message type, so the error response is
sent with the original command message type (e.g.,
`CONNECT_TX_COMMAND = 0`) instead of the correct response type
(`CONNECT_TX_RESPONSE = 1`).
A controller receiving this malformed response would not recognize it
as a response to its command and would eventually time out.
**Fix:** Move `AVB_PACKET_ACMP_SET_MESSAGE_TYPE()` before the
`find_stream()` call so the response type is always set correctly.
## 5. ACMP pending_destroy skips controller cleanup
**File:** `src/modules/module-avb/acmp.c`
**Commit:** `9f4147104`
`pending_destroy()` iterates with `list_id < PENDING_CONTROLLER`
(where `PENDING_CONTROLLER = 2`), which only cleans up
`PENDING_TALKER` (0) and `PENDING_LISTENER` (1) lists, skipping
`PENDING_CONTROLLER` (2). Any pending controller requests leak on
shutdown.
**Fix:** Change `<` to `<=` to include the controller list.
## 6. Legacy AECP handlers read payload at wrong offset
**File:** `src/modules/module-avb/aecp-aem.c`
`handle_acquire_entity_avb_legacy()` and `handle_lock_entity_avb_legacy()`
assign `const struct avb_packet_aecp_aem *p = m;` where `m` is the full
ethernet frame (starting with `struct avb_ethernet_header`). The handlers
then access `p->payload` to read the acquire/lock fields, but this reads
from `m + offsetof(avb_packet_aecp_aem, payload)` instead of the correct
`m + sizeof(avb_ethernet_header) + offsetof(avb_packet_aecp_aem, payload)`.
This causes `descriptor_type` and `descriptor_id` to be read from the
wrong position, leading to incorrect descriptor lookups.
All other AEM command handlers (e.g., `handle_read_descriptor_common`)
correctly derive `p` via `SPA_PTROFF(h, sizeof(*h), void)`.
**Fix:** Change `const struct avb_packet_aecp_aem *p = m;` to properly
skip the ethernet header:
```c
const struct avb_ethernet_header *h = m;
const struct avb_packet_aecp_aem *p = SPA_PTROFF(h, sizeof(*h), void);
```
## 7. Milan LOCK_ENTITY error response uses wrong packet pointer
**File:** `src/modules/module-avb/aecp-aem-cmds-resps/cmd-lock-entity.c`
In `handle_cmd_lock_entity_milan_v12()`, when `server_find_descriptor()`
returns NULL, `reply_status()` is called with `p` (the AEM packet pointer
past the ethernet header) instead of `m` (the full ethernet frame).
`reply_status()` assumes its third argument is the full frame and casts
it as `struct avb_ethernet_header *`. With the wrong pointer, the
response ethernet header (including destination MAC) is corrupted.
**Fix:** Change `reply_status(aecp, ..., p, len)` to
`reply_status(aecp, ..., m, len)`.
## 8. Lock entity re-lock timeout uses wrong units
**File:** `src/modules/module-avb/aecp-aem-cmds-resps/cmd-lock-entity.c`
When a controller that already holds the lock sends another lock request
(to refresh it), the expire timeout is extended by:
```c
lock->base_info.expire_timeout +=
AECP_AEM_LOCK_ENTITY_EXPIRE_TIMEOUT_SECOND;
```
`AECP_AEM_LOCK_ENTITY_EXPIRE_TIMEOUT_SECOND` is `60` (raw seconds),
but `expire_timeout` is in nanoseconds. This adds only 60 nanoseconds
instead of 60 seconds. The initial lock correctly uses
`AECP_AEM_LOCK_ENTITY_EXPIRE_TIMEOUT_SECOND * SPA_NSEC_PER_SEC`.
**Fix:** Multiply by `SPA_NSEC_PER_SEC` to match the nanosecond units.

View file

@ -163,3 +163,42 @@ if valgrind.found()
env : valgrind_env,
timeout_multiplier : 3)
endif
if build_module_avb
avb_test_inc = [pwtest_inc, include_directories('../src/modules')]
avb_module_sources = [
'../src/modules/module-avb/avb.c',
'../src/modules/module-avb/adp.c',
'../src/modules/module-avb/acmp.c',
'../src/modules/module-avb/aecp.c',
'../src/modules/module-avb/aecp-aem.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-available.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-get-set-control.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-get-set-name.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-get-set-clock-source.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-get-set-sampling-rate.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-deregister-unsolicited-notifications.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-register-unsolicited-notifications.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-get-set-stream-format.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-lock-entity.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/cmd-get-set-configuration.c',
'../src/modules/module-avb/aecp-aem-cmds-resps/reply-unsol-helpers.c',
'../src/modules/module-avb/es-builder.c',
'../src/modules/module-avb/avdecc.c',
'../src/modules/module-avb/descriptors.c',
'../src/modules/module-avb/maap.c',
'../src/modules/module-avb/mmrp.c',
'../src/modules/module-avb/mrp.c',
'../src/modules/module-avb/msrp.c',
'../src/modules/module-avb/mvrp.c',
'../src/modules/module-avb/srp.c',
'../src/modules/module-avb/stream.c',
]
test('test-avb',
executable('test-avb',
['test-avb.c'] + avb_module_sources,
include_directories: avb_test_inc,
dependencies: [spa_dep, pipewire_dep, mathlib, dl_lib, rt_lib],
link_with: pwtest_lib)
)
endif

356
test/test-avb-utils.h Normal file
View file

@ -0,0 +1,356 @@
/* AVB test utilities */
/* SPDX-FileCopyrightText: Copyright © 2026 PipeWire contributors */
/* SPDX-License-Identifier: MIT */
#ifndef TEST_AVB_UTILS_H
#define TEST_AVB_UTILS_H
#include <pipewire/pipewire.h>
#include "module-avb/internal.h"
#include "module-avb/packets.h"
#include "module-avb/adp.h"
#include "module-avb/acmp.h"
#include "module-avb/mrp.h"
#include "module-avb/msrp.h"
#include "module-avb/mvrp.h"
#include "module-avb/mmrp.h"
#include "module-avb/maap.h"
#include "module-avb/aecp.h"
#include "module-avb/aecp-aem.h"
#include "module-avb/aecp-aem-descriptors.h"
#include "module-avb/aecp-aem-state.h"
#include "module-avb/iec61883.h"
#include "module-avb/aaf.h"
#include "module-avb/stream.h"
#include "module-avb/descriptors.h"
#include "module-avb/avb-transport-loopback.h"
#define server_emit_message(s,n,m,l) \
spa_hook_list_call(&(s)->listener_list, struct server_events, message, 0, n, m, l)
#define server_emit_periodic(s,n) \
spa_hook_list_call(&(s)->listener_list, struct server_events, periodic, 0, n)
/**
* Create a test AVB server with loopback transport.
* All protocol handlers are registered. No network access required.
*/
static inline struct server *avb_test_server_new(struct impl *impl)
{
struct server *server;
server = calloc(1, sizeof(*server));
if (server == NULL)
return NULL;
server->impl = impl;
server->ifname = strdup("test0");
server->avb_mode = AVB_MODE_LEGACY;
server->transport = &avb_transport_loopback;
spa_list_append(&impl->servers, &server->link);
spa_hook_list_init(&server->listener_list);
spa_list_init(&server->descriptors);
spa_list_init(&server->streams);
if (server->transport->setup(server) < 0)
goto error;
server->mrp = avb_mrp_new(server);
if (server->mrp == NULL)
goto error;
avb_aecp_register(server);
server->maap = avb_maap_register(server);
server->mmrp = avb_mmrp_register(server);
server->msrp = avb_msrp_register(server);
server->mvrp = avb_mvrp_register(server);
avb_adp_register(server);
avb_acmp_register(server);
server->domain_attr = avb_msrp_attribute_new(server->msrp,
AVB_MSRP_ATTRIBUTE_TYPE_DOMAIN);
server->domain_attr->attr.domain.sr_class_id = AVB_MSRP_CLASS_ID_DEFAULT;
server->domain_attr->attr.domain.sr_class_priority = AVB_MSRP_PRIORITY_DEFAULT;
server->domain_attr->attr.domain.sr_class_vid = htons(AVB_DEFAULT_VLAN);
avb_mrp_attribute_begin(server->domain_attr->mrp, 0);
avb_mrp_attribute_join(server->domain_attr->mrp, 0, true);
/* Add a minimal entity descriptor so ADP can advertise.
* We skip init_descriptors() because it creates streams that
* need a pw_core connection. */
{
struct avb_aem_desc_entity entity;
memset(&entity, 0, sizeof(entity));
entity.entity_id = htobe64(server->entity_id);
entity.entity_model_id = htobe64(0x0001000000000001ULL);
entity.entity_capabilities = htonl(
AVB_ADP_ENTITY_CAPABILITY_AEM_SUPPORTED |
AVB_ADP_ENTITY_CAPABILITY_CLASS_A_SUPPORTED |
AVB_ADP_ENTITY_CAPABILITY_GPTP_SUPPORTED);
entity.talker_stream_sources = htons(1);
entity.talker_capabilities = htons(
AVB_ADP_TALKER_CAPABILITY_IMPLEMENTED |
AVB_ADP_TALKER_CAPABILITY_AUDIO_SOURCE);
entity.listener_stream_sinks = htons(1);
entity.listener_capabilities = htons(
AVB_ADP_LISTENER_CAPABILITY_IMPLEMENTED |
AVB_ADP_LISTENER_CAPABILITY_AUDIO_SINK);
entity.configurations_count = htons(1);
server_add_descriptor(server, AVB_AEM_DESC_ENTITY, 0,
sizeof(entity), &entity);
}
return server;
error:
free(server->ifname);
free(server);
return NULL;
}
static inline void avb_test_server_free(struct server *server)
{
avdecc_server_free(server);
}
/**
* Inject a raw packet into the server's protocol dispatch.
* This simulates receiving a packet from the network.
*/
static inline void avb_test_inject_packet(struct server *server,
uint64_t now, const void *data, int len)
{
server_emit_message(server, now, data, len);
}
/**
* Trigger the periodic callback with a given timestamp.
* Use this to advance time and test timeout/readvertise logic.
*/
static inline void avb_test_tick(struct server *server, uint64_t now)
{
server_emit_periodic(server, now);
}
/**
* Build an ADP entity available packet.
* Returns the packet size, or -1 on error.
*/
static inline int avb_test_build_adp_entity_available(
uint8_t *buf, size_t bufsize,
const uint8_t src_mac[6],
uint64_t entity_id,
int valid_time)
{
struct avb_ethernet_header *h;
struct avb_packet_adp *p;
size_t len = sizeof(*h) + sizeof(*p);
static const uint8_t bmac[6] = AVB_BROADCAST_MAC;
if (bufsize < len)
return -1;
memset(buf, 0, len);
h = (struct avb_ethernet_header *)buf;
memcpy(h->dest, bmac, 6);
memcpy(h->src, src_mac, 6);
h->type = htons(AVB_TSN_ETH);
p = (struct avb_packet_adp *)(buf + sizeof(*h));
AVB_PACKET_SET_SUBTYPE(&p->hdr, AVB_SUBTYPE_ADP);
AVB_PACKET_SET_LENGTH(&p->hdr, AVB_ADP_CONTROL_DATA_LENGTH);
AVB_PACKET_ADP_SET_MESSAGE_TYPE(p, AVB_ADP_MESSAGE_TYPE_ENTITY_AVAILABLE);
AVB_PACKET_ADP_SET_VALID_TIME(p, valid_time);
p->entity_id = htobe64(entity_id);
return len;
}
/**
* Build an ADP entity departing packet.
*/
static inline int avb_test_build_adp_entity_departing(
uint8_t *buf, size_t bufsize,
const uint8_t src_mac[6],
uint64_t entity_id)
{
struct avb_ethernet_header *h;
struct avb_packet_adp *p;
size_t len = sizeof(*h) + sizeof(*p);
static const uint8_t bmac[6] = AVB_BROADCAST_MAC;
if (bufsize < len)
return -1;
memset(buf, 0, len);
h = (struct avb_ethernet_header *)buf;
memcpy(h->dest, bmac, 6);
memcpy(h->src, src_mac, 6);
h->type = htons(AVB_TSN_ETH);
p = (struct avb_packet_adp *)(buf + sizeof(*h));
AVB_PACKET_SET_SUBTYPE(&p->hdr, AVB_SUBTYPE_ADP);
AVB_PACKET_SET_LENGTH(&p->hdr, AVB_ADP_CONTROL_DATA_LENGTH);
AVB_PACKET_ADP_SET_MESSAGE_TYPE(p, AVB_ADP_MESSAGE_TYPE_ENTITY_DEPARTING);
p->entity_id = htobe64(entity_id);
return len;
}
/**
* Build an ADP entity discover packet.
*/
static inline int avb_test_build_adp_entity_discover(
uint8_t *buf, size_t bufsize,
const uint8_t src_mac[6],
uint64_t entity_id)
{
struct avb_ethernet_header *h;
struct avb_packet_adp *p;
size_t len = sizeof(*h) + sizeof(*p);
static const uint8_t bmac[6] = AVB_BROADCAST_MAC;
if (bufsize < len)
return -1;
memset(buf, 0, len);
h = (struct avb_ethernet_header *)buf;
memcpy(h->dest, bmac, 6);
memcpy(h->src, src_mac, 6);
h->type = htons(AVB_TSN_ETH);
p = (struct avb_packet_adp *)(buf + sizeof(*h));
AVB_PACKET_SET_SUBTYPE(&p->hdr, AVB_SUBTYPE_ADP);
AVB_PACKET_SET_LENGTH(&p->hdr, AVB_ADP_CONTROL_DATA_LENGTH);
AVB_PACKET_ADP_SET_MESSAGE_TYPE(p, AVB_ADP_MESSAGE_TYPE_ENTITY_DISCOVER);
p->entity_id = htobe64(entity_id);
return len;
}
/**
* Build an AECP AEM command packet for injection.
* Returns packet size, or -1 on error.
*/
static inline int avb_test_build_aecp_aem(
uint8_t *buf, size_t bufsize,
const uint8_t src_mac[6],
uint64_t target_guid,
uint64_t controller_guid,
uint16_t sequence_id,
uint16_t command_type,
const void *payload, size_t payload_size)
{
struct avb_ethernet_header *h;
struct avb_packet_aecp_aem *p;
size_t len = sizeof(*h) + sizeof(*p) + payload_size;
if (bufsize < len)
return -1;
memset(buf, 0, len);
h = (struct avb_ethernet_header *)buf;
memcpy(h->dest, (uint8_t[]){ 0x91, 0xe0, 0xf0, 0x01, 0x00, 0x00 }, 6);
memcpy(h->src, src_mac, 6);
h->type = htons(AVB_TSN_ETH);
p = SPA_PTROFF(h, sizeof(*h), void);
AVB_PACKET_SET_SUBTYPE(&p->aecp.hdr, AVB_SUBTYPE_AECP);
AVB_PACKET_AECP_SET_MESSAGE_TYPE(&p->aecp, AVB_AECP_MESSAGE_TYPE_AEM_COMMAND);
AVB_PACKET_AECP_SET_STATUS(&p->aecp, 0);
AVB_PACKET_SET_LENGTH(&p->aecp.hdr, payload_size + 12);
p->aecp.target_guid = htobe64(target_guid);
p->aecp.controller_guid = htobe64(controller_guid);
p->aecp.sequence_id = htons(sequence_id);
AVB_PACKET_AEM_SET_COMMAND_TYPE(p, command_type);
if (payload && payload_size > 0)
memcpy(p->payload, payload, payload_size);
return len;
}
/**
* Create a test AVB server in Milan v1.2 mode with loopback transport.
* The entity descriptor is properly sized for Milan state (lock, unsol).
*/
static inline struct server *avb_test_server_new_milan(struct impl *impl)
{
struct server *server;
server = calloc(1, sizeof(*server));
if (server == NULL)
return NULL;
server->impl = impl;
server->ifname = strdup("test0");
server->avb_mode = AVB_MODE_MILAN_V12;
server->transport = &avb_transport_loopback;
spa_list_append(&impl->servers, &server->link);
spa_hook_list_init(&server->listener_list);
spa_list_init(&server->descriptors);
spa_list_init(&server->streams);
if (server->transport->setup(server) < 0)
goto error;
server->mrp = avb_mrp_new(server);
if (server->mrp == NULL)
goto error;
avb_aecp_register(server);
server->maap = avb_maap_register(server);
server->mmrp = avb_mmrp_register(server);
server->msrp = avb_msrp_register(server);
server->mvrp = avb_mvrp_register(server);
avb_adp_register(server);
avb_acmp_register(server);
server->domain_attr = avb_msrp_attribute_new(server->msrp,
AVB_MSRP_ATTRIBUTE_TYPE_DOMAIN);
server->domain_attr->attr.domain.sr_class_id = AVB_MSRP_CLASS_ID_DEFAULT;
server->domain_attr->attr.domain.sr_class_priority = AVB_MSRP_PRIORITY_DEFAULT;
server->domain_attr->attr.domain.sr_class_vid = htons(AVB_DEFAULT_VLAN);
avb_mrp_attribute_begin(server->domain_attr->mrp, 0);
avb_mrp_attribute_join(server->domain_attr->mrp, 0, true);
/* Add Milan-sized entity descriptor with lock/unsol state */
{
struct aecp_aem_entity_milan_state entity_state;
memset(&entity_state, 0, sizeof(entity_state));
entity_state.state.desc.entity_id = htobe64(server->entity_id);
entity_state.state.desc.entity_model_id = htobe64(0x0001000000000001ULL);
entity_state.state.desc.entity_capabilities = htonl(
AVB_ADP_ENTITY_CAPABILITY_AEM_SUPPORTED |
AVB_ADP_ENTITY_CAPABILITY_CLASS_A_SUPPORTED |
AVB_ADP_ENTITY_CAPABILITY_GPTP_SUPPORTED);
entity_state.state.desc.talker_stream_sources = htons(1);
entity_state.state.desc.talker_capabilities = htons(
AVB_ADP_TALKER_CAPABILITY_IMPLEMENTED |
AVB_ADP_TALKER_CAPABILITY_AUDIO_SOURCE);
entity_state.state.desc.listener_stream_sinks = htons(1);
entity_state.state.desc.listener_capabilities = htons(
AVB_ADP_LISTENER_CAPABILITY_IMPLEMENTED |
AVB_ADP_LISTENER_CAPABILITY_AUDIO_SINK);
entity_state.state.desc.configurations_count = htons(1);
server_add_descriptor(server, AVB_AEM_DESC_ENTITY, 0,
sizeof(entity_state), &entity_state);
}
return server;
error:
free(server->ifname);
free(server);
return NULL;
}
#endif /* TEST_AVB_UTILS_H */

4017
test/test-avb.c Normal file

File diff suppressed because it is too large Load diff