[RSDK-9927] increase min data needed before attempting parse firmware header #412
Conversation
| @@ -64,7 +67,14 @@ use {bincode::Decode, futures_lite::AsyncWriteExt}; | |||
| const CONN_RETRY_SECS: u64 = 1; | |||
| const NUM_RETRY_CONN: usize = 3; | |||
| const DOWNLOAD_TIMEOUT_SECS: u64 = 30; | |||
| const SIZEOF_APPDESC: usize = 256; | |||
There was a problem hiding this comment.
looks like this is still being referenced in L. 481 (in the native case), probably meant to replace with FIRMWARE_HEADER_SIZE?
| #[cfg(feature = "esp32")] | ||
| static FIRMWARE_HEADER_SIZE: Lazy<usize> = Lazy::new(|| { | ||
| std::mem::size_of::<esp_image_header_t>() | ||
| + std::mem::size_of::<esp_image_segment_header_t>() |
There was a problem hiding this comment.
IIUC you are using this to compute whether the frame you got contains enough data to get you to the esp_app_desc_t. But is there an assumption here that the first segment will be the DROM segment which contains the esp_app_desc_t? Could the segments be in another order?
There was a problem hiding this comment.
I don't believe so, not for something valid that goes directly into an app partition at least. This was pulled directly from esp-idf-svc's FirmwareInfoLoader.
Found a seemingly relevant commit comment as well.
The esp_app_desc_t is located in fixed place in start of ROM secotor. It is located after structures esp_image_header_t and esp_image_segment_header_t.
There was a problem hiding this comment.
side note: this is probably why the outputs of my handrolled struct at the top was giving me strange results. I was parsing from the image header and segment header up front, not the app_desc_t like I thought
There was a problem hiding this comment.
Thanks for checking. It might be worth linking to those resources in a comment on this constant.
Meanwhile, were there any checks that you wanted to make with that struct that you had backed away from doing because you weren't getting sensible results? Would you want to re-introduce them now that this issue has been identified and fixed?
There was a problem hiding this comment.
not at the minute. There's already an embedded ticket that's backlogged. I'll add this thread to the ticket for context when we get to it.
| @@ -64,7 +67,14 @@ use {bincode::Decode, futures_lite::AsyncWriteExt}; | |||
| const CONN_RETRY_SECS: u64 = 1; | |||
| const NUM_RETRY_CONN: usize = 3; | |||
| const DOWNLOAD_TIMEOUT_SECS: u64 = 30; | |||
| const SIZEOF_APPDESC: usize = 256; | |||
| @@ -442,7 +452,7 @@ impl<S: OtaMetadataStorage> OtaService<S> { | |||
| total_downloaded += data.len(); | |||
|
|
|||
| if !got_info { | |||
| if total_downloaded < SIZEOF_APPDESC { | |||
| if total_downloaded < *FIRMWARE_HEADER_SIZE { | |||
There was a problem hiding this comment.
Micro-nit: Above, we set the max frame size to 16k. If, somehow FIRMWARE_HEADER_SIZE was > 16k, then this would always fail. Maybe the max frame size should be max(16k, FIRMWARE_HEADER_SIZE).
There was a problem hiding this comment.
I think it's safe to assume (as we're only supporting esp32 at the moment) that a valid header will always be <16K.
We could calculate it out (esp_image_header_t + esp_image_segment_header_t + esp_app_desc_t) and set a magic number accordingly, but I think sizeof provides more context on what we're actually doing. I used 1024 for non-esp32 targets arbitrarily since it's greater than the three types above and it won't have access to the esp_idf_* types.
Also, I'm reluctant to set something greater than the 16K as max_frame_size since its documentation stated it was the max and would need to look deeper to see what the effects of setting it higher would be inside hyper.
| #[cfg(feature = "esp32")] | ||
| static FIRMWARE_HEADER_SIZE: Lazy<usize> = Lazy::new(|| { | ||
| std::mem::size_of::<esp_image_header_t>() | ||
| + std::mem::size_of::<esp_image_segment_header_t>() |
There was a problem hiding this comment.
Thanks for checking. It might be worth linking to those resources in a comment on this constant.
Meanwhile, were there any checks that you wanted to make with that struct that you had backed away from doing because you weren't getting sensible results? Would you want to re-introduce them now that this issue has been identified and fixed?
No description provided.