hci: add unit tests, and correct what they found - #476
Conversation
|
UPDATE: tested and working on |
|
Given the nature of the refactor you might want to stack refactors on multiple PRs. The ble package makes sense and probably should have its own PR for clarity |
@acouvreur an excellent idea, I am working on that right now... |
Are youbsure you are willing to go forward with this whole refactor ? This is a huge set of breaking change. I'm not against it, far from it. But I recall having some conversation with you about the public API of this repository. If we are willing to go forward with these refactors, that would be a good opportunity to discuss some other refactors before releasing. |
|
I do not think that there are any breaking changes for external users. |
deadprogram
changed the title
|
Good call. The stack is now three pull requests, and each commit boundary
Review order is 477, 478, then 476. GitHub retargets each one to |
|
@deadprogram I would recommend you to create a stacked pull request, so that rebasing is done automatically. Right now the merge of #477 is now blocking #478 for conflicts. With the stacked PR this will be avoided. 
|
A test for a timeout path had to wait 3 or 10 seconds of real time. The command timeout, the response timeout and the delay between polls are now fields, so a test shortens them and the path finishes at once. The defaults are unchanged. The fields stay unexported, because the defaults are the only supported setting. The command timeout compared (now-start)/int64(time.Second) > 3, which fires somewhere between 3 and 4 seconds. It is now time.Since(start) against the field.
The protocol code had no tests, because the build tags kept it off the host. It now has 190 test cases and 87 percent statement coverage, with a fake transport in two flavours: a stream one that models a UART, including a packet split over two reads, and a packet oriented one that models the CYW43439 ring, including the read that does not fit. The tests found six defects, all in code that has shipped: Find Information Response treated its format byte as an entry length, so a well formed response from any peer sliced past the end of the buffer and panicked. Read By Type Response and Read By Group Type Response took their entry length off the wire without a bounds check, so a bad length read past the end, and a zero length looped forever. No ATT opcode checked that the protocol data unit was long enough before reading it, so a two byte one panicked. handleData now checks the minimum length for the opcode first, and each unmarshaller checks its own input. A sweep over every opcode at every length up to 24 bytes covers this, and the same sweep runs over every HCI event, every LE subevent and every L2CAP signalling code. ReadBdAddr copied the wrong bytes, because cmdResponse started at the event length byte rather than at the return parameters. Address returned 0a:01:09:10:00:11 instead of the controller address. CommandResponse now returns the return parameters alone, which is what a caller wants. ReadLEBufferSize read the raw read buffer rather than the response, so it took the packet type and event code as the packet length and raised the maximum MTU to 3588. Nothing calls it yet, so the overflow that would follow was still latent. It now reads the response and clamps to MaximumMTU, which is what the response buffers are sized for. MTUReq asked for the connection MTU, which is zero before an exchange. It now asks for the largest MTU this stack accepts. att.mtu was never assigned, so MTU and GetMTU always returned zero, and the server used zero as the size budget for a discovery response and so sent one attribute per round trip. It now starts at the ATT default of 23 and follows the exchange. L2CAP discarded the error from unmarshalling a connection parameter update request, so a truncated one was answered as accepted with zeroed parameters, and handleData read the first two bytes without checking that they were there. The Find Information Response that this stack sends picks its format from the attribute type rather than the UUID width, so a database whose first match is a service declaration answers with format 2 and 16 bit UUIDs. That is a protocol correction rather than a crash, so it is marked with a TODO and left for its own change. Also removes three serialisers that nothing called.
go test with no arguments only tested the root package, so the new ble and hci packages were never tested. It now names the three packages. It cannot be ./... , because the baremetal examples import machine and do not build on the host. The Linux job also runs the race detector, because ATT uses a mutex and a channel across goroutines. make smoketest-hci builds the four HCI transports for a faster loop than the whole matrix, and adds two bledebug builds, which nothing covered.
5340498 to
d8c1e25
Compare
|
@acouvreur I created a stack for the remaining two, sorry I did not do that initially. |
|
Ready for stacked review @acouvreur |
2 participants
Adds unit tests to the HCI protocol stack, and corrects the eight defects that
they found.
This is the last of three stacked pull requests. It builds on #478
hci: move the protocol stack into a subpackage, which took the build tags offthe protocol code and made these tests possible.
Tests
219 test cases, 87 percent statement coverage, race clean. The fake transport
comes in two flavours: a stream one that models a UART, including a packet
split over two reads, and a packet oriented one that models the CYW43439 ring,
including the read that does not fit. There is also a sweep over every ATT
opcode, every HCI event, every LE subevent and every L2CAP signalling code at
every length up to 24 bytes, which is what found most of the following.
Defects the tests found
All of these are in code that has shipped.
entry length, so a well formed response from any peer sliced past the end of
the buffer. Reachable over the air.
off the wire with no bounds check. A bad length read past the end, and a
zero length looped forever.
data unit panicked.
ReadBdAddrreturned the wrong address.cmdResponsestarted at theevent length byte rather than at the return parameters, so
Address()returned
0a:01:09:10:00:11.CommandResponse()now returns the returnparameters alone.
ReadLEBufferSizeread the raw packet buffer, taking the packet type andevent code as the packet length, and raised the maximum MTU to 3588 against
response buffers sized for 248. Nothing calls it yet, so the overflow was
latent.
MTUReqasked for an MTU of zero, because it sent the connection MTUbefore any exchange.
att.mtuwas never assigned, soGetMTUalways returned zero and theserver used zero as the size budget for a discovery response, sending one
attribute per round trip.
it as accepted with zeroed parameters.
One protocol defect is not fixed here, because it is a behaviour change on
the peripheral discovery path that wants hardware validation: the Find
Information Response that this stack sends picks its format from the attribute
type rather than the UUID width, so a database whose first match is a service
declaration answers with format 2 and 16 bit UUIDs. It is marked with a TODO.
Three commits, in order
hci: make the timeouts settable so that tests do not wait, so a timeouttest does not wait 10 seconds.
hci: add unit tests, and correct what they found.ci: test every package, and add a faster HCI smoke test. A newmake smoketest-hcibuilds the four HCI transports and twobledebugconfigurations, which nothing covered before.
Size
-size=short, against #478:hci hci_uartThe growth is the bounds checks. RAM is flat.
Verification
go vet,go testandgo test -raceover./ ./ble ./hci, plusmake smoketest-tinygo,smoketest-linux,smoketest-windowsandsmoketest-hci.Run on hardware.
examples/discover, which is the central path, works on oneninafw board, one pico-w and one esp32, against
dev, against the move andagainst the tests.
examples/heartrate, which is the peripheral path, was alsochecked on a ninafw board with a host as the central: the service and
characteristic discovery, the MTU and the read permissions are the same in all
three. The peripheral path is not yet checked on pico-w or esp32.