libblueman: fix HCI/SDP resource leaks and an ifname over-read, add a 100%-covered C test suite

[geraldo-netto]

Summary

Hardens the native helper module/libblueman.c (the C backing the _blueman Cython extension) and adds the module's first C test suite, at 100% line coverage. The C here is delicate: it allocates kernel/SDP structures, opens HCI/RFCOMM sockets, and walks BlueZ-owned linked lists, all on manual cleanup paths. Each finding below is a real resource or memory-safety bug on those paths, fixed conservatively so the success paths behave exactly as before.

The PR is split into five commits, each reviewable on its own: the original fix, the test suite, and three follow-up commits from a second review pass over the branch (state-machine and lifecycle issues described under "Second review pass" below).

Why this needs care (the C delicateness)

Most of these functions follow the classic C pattern of "acquire a resource, do work, clean up on every exit". The bugs are all in the exit paths — the cases that only fire when a syscall or allocation fails, which is exactly where coverage and review usually thin out. Three properties make this code easy to get wrong:

1. Ownership is implicit. connection_init opens an HCI device and hands its fd to the caller via conn_info_handles.dd, to be closed later by connection_close. But that ownership only transfers on the success path; on any earlier failure the fd belongs to connection_init and must be closed there. The original closed it nowhere on those paths.
2. Cleanup is duplicated per branch. get_rfcomm_channel frees its three SDP lists only inside the search-error branch. The happy path returns without freeing them. Because the lists are allocated by sdp_list_append (BlueZ-owned memory), nothing else reclaims them — every successful lookup leaked.
3. Fixed-width copies assume max-length input. _create_bridge used memcpy(ifr.ifr_name, name, IFNAMSIZ), copying a fixed 16 bytes regardless of the actual string length — an out-of-bounds read for any shorter name. The sibling _destroy_bridge already did this correctly with strncpy; the two had simply drifted.

Because these only manifest on error paths or under tooling (ASan), they are invisible in normal use and in a quick read. That is precisely why the test suite builds a harness that forces every error path and runs the whole thing under AddressSanitizer + LeakSanitizer.

Findings and fixes

High — resource leaks

connection_init leaked the HCI device on every post-open error. After dd = hci_open_dev(...) succeeds, both the malloc failure and the HCIGETCONNINFO ioctl failure jumped to cleanup that only free(cr) — dd was never hci_close_dev'd and was not returned, so the caller could not close it either. Fix: close dd on all error paths; null it out once ownership transfers to the caller's conn_info_handles so the shared cleanup skips it.

get_rfcomm_channel leaked all three SDP lists on success. search_list, attrid_list, and response_list were freed only on the sdp_service_search_attr_req error branch. Fix: a single cleanup label frees all three (and closes the session) for both success and error, matching how the per-record data is already freed inside the loop.

Medium — memory safety

_create_bridge out-of-bounds read. memcpy(ifr.ifr_name, name, IFNAMSIZ) over-reads when name is shorter than IFNAMSIZ. Fix: zero the ifreq and use a bounded copy, matching _destroy_bridge.

errno clobbering on error returns. Several error paths ran return -errno after close(sock), which can overwrite errno. Fix: capture errno into a local before every close() on the error returns.

Low — input validation and robustness

• Validate str2ba in connection_init, create_rfcomm_device, and get_rfcomm_channel instead of proceeding on an indeterminate address. Adds ERR_INVALID_ADDRESS (-16), mapped in _blueman.pyx's error table so the Python side raises a clear message rather than KeyError.
• _destroy_bridge: bound strncpy to IFNAMSIZ - 1.
• find_conn: cast the callback's long arg through intptr_t rather than directly to a pointer.
• const-correctness on read-only pointer parameters; align the header's parameter name with the definition.

Second review pass (follow-up commits)

A second review of the branch, focused on state-machine and lifecycle behavior, found and fixed four more issues:

Forward-delay failure is non-fatal again, now documented. An earlier revision surfaced a SIOCDEVPRIVATE (forward-delay) failure as an error — but at that point SIOCBRADDBR has already succeeded, so failing there leaves a half-created bridge behind, and BRCTL_SET_BRIDGE_FORWARD_DELAY via SIOCDEVPRIVATE is a legacy ioctl interface that a kernel may reject while bridge creation works. Since NetConf tolerates EEXIST on retry, a retry would silently skip the delay anyway. The original non-fatal semantics are restored with a comment in the code explaining why.

Bounded name buffers for SIOCBRADDBR/SIOCBRDELBR. The kernel always does copy_from_user(buf, uarg, IFNAMSIZ) for these ioctls, so passing the raw string pointer permits a read past a shorter allocation — the same class as the ifr_name over-read fixed above. Both bridge functions now copy the name into a zero-padded char ifname[IFNAMSIZ] local and pass that everywhere.

No strncpy. The bounded copy is a small documented helper (copy_ifname) rather than strncpy, which the Linux kernel deprecated (Documentation/process/deprecated.rst) because it does not guarantee NUL-termination when the source fills the buffer — and, after a six-year, 362-commit conversion effort, removed from the kernel entirely for v7.2. The helper implements the strscpy_pad() semantics the bridge ioctls need — always-terminated copy into an explicitly zeroed IFNAMSIZ buffer — using ISO C memchr/memcpy, since userspace has no portable strscpy.

connection_close is idempotent. It now no-ops when dd < 0 and invalidates dd after closing, so a double close (or a close before a successful connection_init) can never touch a recycled file descriptor.

conn_info use-after-free in _blueman.pyx. __init__ stored a char* into a temporary bytes object that is freed when __init__ returns, so init() passed a dangling pointer to connection_init. The bytes object is now kept alive on the instance. __init__ also initializes ci.dd = -1, so deinit() before a successful init() is a no-op instead of closing fd 0.

Plus defense in depth: find_conn clamps the ioctl-returned conn_num to the 10 entries it allocated, so a broken kernel contract can never walk the scan loop out of bounds.

ABI / compatibility

No change to existing behavior on success paths. The ABI additions are the new ERR_INVALID_ADDRESS code (mapped in _blueman.pyx) and connection_close taking a non-const pointer so it can invalidate the descriptor. Error-path semantics are unchanged except that every resource is now released.

Testing

test/module/test_libblueman.c #includes the translation unit (to reach the static helpers) and redirects every libc and libbluetooth call with ld --wrap, so all success and error paths run with no Bluetooth adapter and no root. The mocks count socket / HCI / SDP allocations, and the tests assert each resource is released — this is what proves the leak and OOB fixes rather than merely asserting return codes. Built under AddressSanitizer + LeakSanitizer, so a regression in any cleanup path aborts the run.

• run_tests.sh builds, runs (ASan), and reports gcov line coverage: 100% (243/243), 92 checks, 0 failures.
• test/module/test_libblueman_c.py runs the harness under the normal Python test runner and skips gracefully when the C toolchain, ASan, or BlueZ headers are unavailable, so it never falsely fails minimal CI environments.
• Coverage spans bridge create/destroy (every ioctl failure point), find_conn (match / no-match / ioctl / malloc failure / hostile conn_num), the full connection_init lifecycle including the device-leak error paths and idempotent close, rfcomm list/create/release, and get_rfcomm_channel including SDP record parsing and cleanup — plus fuzz passes over odd interface names and Bluetooth addresses (the address/name strings that would have tripped the old over-read).
• The hostile conn_num case is a true ASan-backed regression test: with the clamp removed, the scan loop reads past the allocation and ASan aborts the run.

gcc -Wall -Wextra is clean; cppcheck reports nothing on the file.

Deliberately left as-is (optional follow-up cleanups)

These are not bugs and are not part of this PR; noting them for reviewers since they touch the same file. They are code-quality cleanups, not performance wins — the module is syscall/IO-bound, so none of them changes measurable runtime.

• Redundant helper — __tv_to_jiffies({0, 0}). With a zero timeval it always returns 0, so the bridge forward delay could be set with a plain args[1] = 0 and the helper dropped. It is kept here for clarity and because it is the one trivially unit-testable pure function in the file.
• printf from a library helper. get_rfcomm_channel writes to stdout from code that is linked into the Python extension — the wrong layer, and stdio is the only non-trivial cost in an otherwise cheap function. Replacing these with proper logging (or removing them) would be a real cleanup and would stop stray output reaching the app's stdout.
• Missing early exit. Once the RFCOMM channel is found, the loop still walks every remaining record / protocol / attribute. A break after the match would be clearer and skip the dead iterations, though SDP responses are small enough that the saving is unmeasurable.

Happy to fold any of these into this PR or a follow-up if preferred.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[geraldo-netto]

@cschramm @infirit I have updated this PR because kernel 7.2 recently removed strncpy, see the commit below:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1a3746ccbb0a97bed3c06ccde6b880013b1dddc1