Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

FSService: Initialise WriteResponse::status on all paths - #2457

#2457
Open
JoeStrout wants to merge 1 commit into
InfiniTimeOrg:mainInfiniTimeOrg/InfiniTime:mainfrom
JoeStrout:fix-fsservice-write-statusJoeStrout/InfiniTime:fix-fsservice-write-statusCopy head branch name to clipboard
Open

FSService: Initialise WriteResponse::status on all paths#2457
JoeStrout wants to merge 1 commit into
InfiniTimeOrg:mainInfiniTimeOrg/InfiniTime:mainfrom
JoeStrout:fix-fsservice-write-statusJoeStrout/InfiniTime:fix-fsservice-write-statusCopy head branch name to clipboard

Conversation

@JoeStrout

Copy link
Copy Markdown

WriteResponse has no default member initialisers and both write handlers assigned status on only one branch, so the other returned uninitialised stack memory to the client over BLE:

  • commands::WRITE assigned status only on success, so a failed FileOpen reported garbage. (The ternary was also dead code, as res is known to be 0 inside that branch.)
  • commands::WRITE_DATA assigned status only on failure, so every successful chunk reported garbage, which readily looks like a negative LittleFS error code.

This change value-initialises both, matching DelResponse, MKDirResponse and ListDirResponse in the same file, and assign status on both paths.

The visible symptom is a companion app (like InfiniTimeExplorer) reporting a file transfer error ("Error creating file [0]") even when writing the file correctly.

WriteResponse has no default member initialisers and both write handlers
assigned status on only one branch, so the other returned uninitialised
stack memory to the client over BLE:

- commands::WRITE assigned status only on success, so a failed FileOpen
  reported garbage. The ternary was also dead code, as res is known to be
  0 inside that branch.
- commands::WRITE_DATA assigned status only on failure, so every
  successful chunk reported garbage, which readily looks like a negative
  LittleFS error code.

Value-initialise both, matching DelResponse, MKDirResponse and
ListDirResponse in the same file, and assign status on both paths.

The visible symptom is a companion app reporting a file transfer error
against a watch that is writing the file correctly.
@github-actions

Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 385248B 16B
data 944B 0B
bss 22640B 0B

Run in InfiniEmu

@JoeStrout

Copy link
Copy Markdown
Author

Note: ReadResponse and the other response structs are also declared without {}, so their padding fields go out over the air as uninitialized stack bytes. Harmless functionally, but it is a small information leak. It might be worth fixing just on general principles — happy to add this to my PR if you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Morty Proxy This is a proxified and sanitized view of the page, visit original site.