ClusterLabs / ClusterLabs/libqb

Alignment on own struct types violated by ringbuffer

Open
#483 3 comments 0 reactions 1 assignee Claimed by @chrissie-c View on GitHub
Dominant language
C
Stars
171
Forks
103
PR merge metrics
No merged PRs in 30d

Description

Some libqb structs enforce specific alignment like 8 bytes for `struct qb_ipc_request_header`, but it seems that not all the code path handling such a struct enforces that, at least when using the shared memory variant, and the library can end up passing unaligned pointers around, e.g., to the msg_process callback, resulting in undefined behavior when accessing its struct members.

As example use the following truncated msg_process callback:
```C
static int32_t s1_msg_process_fn(
qb_ipcs_connection_t *c,
void *data,
size_t size
) {
struct qb_ipc_request_header *req_pt = (struct qb_ipc_request_header *) data;
int32_t request_id = req_pt->id;
int32_t request_size = req_pt->size;
// ...
return 0;
}
```

Which is exercised on the client side by doing something along the lines of
```C
int iov_len = 2;
struct iovec iov[iov_len];

struct qb_ipc_request_header req_header = {
.id = msgid,
.size = sizeof(struct qb_ipc_request_header) + len,
};

iov[0].iov_base = (char *)&req_header;
iov[0].iov_len = sizeof(req_header);
iov[1].iov_base = dataptr;
iov[1].iov_len = len;

int32_t timeout = -1;
int res = qb_ipcc_sendv_recv(conn, iov, iov_len, reply_buffer, sizeof(reply_buffer), timeout);
```

Just for the record: The cast struct enforces 8byte alignment on members and itself:
```C
struct qb_ipc_request_header {
int32_t id __attribute__ ((aligned(8)));
int32_t size __attribute__ ((aligned(8)));
} __attribute__ ((aligned(8)));
```

Compiling above with ASan & (for this issue the actual relevant) UBSan, e.g. for a common Makefile it'd look something like:
```
CFLAGS += -fsanitize=address,undefined
LDFLAGS += -fsanitize=address,undefined -static-libasan -static-libubsan
```

one then sometimes (not always!) gets an `member access within misaligned address` error reported:

```
server.c:184:10: runtime error: member access within misaligned address 0x7f4f9d0a6094 for type 'struct qb_ipc_request_header', which requires 8 byte alignment
0x7f4f9d0a6094: note: pointer points here
a1 a1 a1 a1 06 00 00 00 00 00 00 00 21 00 00 00 00 00 00 00 68 61 2f 72 65 73 6f 75 72 63 65 73
^
server.c:185:10: runtime error: member access within misaligned address 0x7f4f9d0a6094 for type 'struct qb_ipc_request_header', which requires 8 byte alignment
0x7f4f9d0a6094: note: pointer points here
a1 a1 a1 a1 06 00 00 00 00 00 00 00 21 00 00 00 00 00 00 00 68 61 2f 72 65 73 6f 75 72 63 65 73
^
```

After looking through the whole call chain and checking how our side and libqb sides handles pointers, it seems that misalignment comes from the libqb ring buffer implementation, namely `qb_rb_chunk_step` which steps the write pointer **always** by `sizeof(uint32_t)`, i.e., four bytes, breaking the alignment rules of the `qb_ipc_request_header` struct.

If the base alignment of most (all?) public types really should be 8 bytes then we could simply increment the write_puffer always by 8 bytes.

But IMO, the 8 byte alignment is rather odd in the first place and might waste quite a bit of (CPU cache) space, albeit I did not really analyze that part closely. So, is there an actual reason for keeping those? Besides ABI breakage causing churn, naturally.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.