fw/services: answer a put_bytes install with its own token - #1987
Open
zunda-pixel wants to merge 1 commit into
Open
fw/services: answer a put_bytes install with its own token#1987zunda-pixel wants to merge 1 commit into
zunda-pixel wants to merge 1 commit into
Conversation
The response to a PutBytes INSTALL carried token 0 instead of the token the phone sent. prv_cleanup_and_send_response reads s_pb_state.token, and the COMMIT that always precedes an install has already run prv_cleanup, which resets the transfer state. prv_do_install validates the phone's token against s_ready_to_install, so the right value was in hand — it simply never reached the response. Zero was guaranteed rather than racy: prv_is_valid_command_for_current_state admits PutBytesInstall only from PutBytesIdle, so an install is always answered with a torn-down transfer state. prv_cleanup already preserves type across that teardown, with a NOTE saying it does so because the install handler reads it afterwards; the token needed the same care and did not get it. A phone that matches a response to the request it sent therefore never sees the install finish: the transfer reaches 100%, the watch writes the object, and the update hangs. Observed on a Pebble Time 2 (obelix_pvt) running v4.36.2 from a third-party companion app, which now has to ignore the token on this one message. Both the ACK and the NACK paths answer with the install's own token now, which also makes install responses self-consistent: the state-error path already echoed the request token through prv_fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: zunda pixel <zunda.dev@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The response to a PutBytes
INSTALL(0x05) carries token0instead of the token the phone sent. Both the ACK and the NACK paths now answer with the install's own token.Why
prv_cleanup_and_send_responsereadss_pb_state.token, but theCOMMIT(0x03) that always precedes an install has already gone throughprv_cleanup, which resets the transfer state:That
NOTEis the bug in miniature: the "install runs after cleanup" invariant was already known,typewas carried across it, andtokenwas not.Zero is guaranteed rather than racy —
prv_is_valid_command_for_current_stateadmitsPutBytesInstallonly fromPutBytesIdle, so an install is always answered with a torn-down state. The value that should be echoed was in hand the whole time:prv_do_install(token)looks the phone's token up ins_ready_to_install[]and NACKs a mismatch.A phone that matches a response to the request it sent therefore never sees the install finish: the transfer reaches 100 %, the watch writes the object, and the update hangs.
How it was found
Observed on a Pebble Time 2 (
obelix_pvt) running v4.36.2, from a third-party iOS companion app I maintain, which has to ignore the token on this one message to get past it.libpebble3 hit the same wall — the cookie check in
PutBytesSession.sendInstallis commented out with// TODO this fired?, whilesendPutandsendCommitkeep theirs. coredevices/mobileapp#398 restores it.Tests
Two regression tests added to
tests/fw/services/test_put_bytes.c, one per path:install_ack_carries_the_install_tokeninstall_nack_carries_the_install_tokenReverting only the
src/hunk makes both fail with the reported symptom (0 NOT == <token>), so they test the bug rather than the new code. Locally,ctest --test-dir build-test -R put_bytes:Notes
prv_fail, while the success and unknown-token paths answered 0.prv_cleanup_and_send_responsekeeps its signature and callers; the new_with_tokenvariant is used only by the two install paths. Preservingtokenacrossprv_cleanupthe waytypeis preserved would have leaked stale state into every other path that readss_pb_state.token.AI use
Written with Claude Code (Claude Opus 5); disclosed by the
Co-Authored-Bytrailer on the commit, per CONTRIBUTING's generative AI section. I have reviewed the change and the firmware paths it describes, and am happy to answer questions about either.Seen end to end in QEMU
Built
qemu_emerytwice, differing only in this hunk, and ran the same firmware install against each from a client that logs the token of every put_bytes answer. Run-length encoded, since a transfer acknowledges every chunk with the same token:This branch
mainSo each install is answered with its own token rather than with zero, which is what this changes.
One thing in the
mainrun I cannot account for and would rather flag than explain away: only the first install was answered at all, and the second drew nothing, so the client gave up after its 10s deadline. A real Pebble Time 2 on v4.36.2 answered both installs 32ms apart in the same client's log earlier today, so the emulator and the hardware do not agree here and I do not know which way the difference runs. It has no bearing on the token, which is what the two host tests pin and what the diff is about.