Skip to content

Fix logic error that loses the remainder chunk #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jerrysxie wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix logic error that loses the remainder chunk #27

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
faa167d
Fix logic error that loses the remainder chunk
jerrysxie Dec 19, 2025
bbf2083
Add check for image to be at least 2 chunks
jerrysxie Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 52 additions & 22 deletions src/host.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::protocol_definitions::{
FW_UPDATE_FLAG_LAST_BLOCK,
};
use crate::writer::{CfuWriterAsync, CfuWriterError};
use crate::{trace, CfuImage, DataChunk};
use crate::{error, trace, CfuImage, DataChunk};

/// CfuHostStates trait defines behavior needed for a Cfu Host to process available Cfu Offers
/// and send the appropriate commands to the Cfu Client to update the components
Expand Down Expand Up @@ -77,6 +77,15 @@ impl<W: CfuWriterAsync> CfuUpdateContent<W> for CfuUpdater {
cmpt_id: ComponentId,
base_offset: usize,
) -> Result<FwUpdateContentResponse, CfuProtocolError> {
let total_bytes: usize = image.get_total_size();

if total_bytes <= DEFAULT_DATA_LENGTH {
error!(
"image size less than or equal to chunk size, we need at least for 2 chunks for first and last block"
);
return Err(CfuProtocolError::WriterError(CfuWriterError::Other));
}

// Build update offer command
let updateoffercmd_bytes = [0u8; 16];
let mut offer_resp = [0u8; 16];
Expand All @@ -93,53 +102,74 @@ impl<W: CfuWriterAsync> CfuUpdateContent<W> for CfuUpdater {
return Err(CfuProtocolError::CfuContentUpdateResponseError(status));
}

let total_bytes: usize = image.get_total_size();
let chunk_size = DEFAULT_DATA_LENGTH;
let num_chunks = total_bytes / chunk_size;
let remainder = total_bytes % chunk_size;

// Read and process data in chunks so as to not over-burden memory resources
let mut resp: FwUpdateContentResponse =
FwUpdateContentResponse::new(0, CfuUpdateContentResponseStatus::ErrorInvalid);

for i in 0..num_chunks {
let mut chunk = [0u8; DEFAULT_DATA_LENGTH];
let address_offset = i * DEFAULT_DATA_LENGTH + base_offset;

image
.get_bytes_for_chunk(&mut chunk, address_offset)
.await
.map_err(|_| CfuProtocolError::WriterError(CfuWriterError::StorageError))?;

let r = match i {
0 => {
image
.get_bytes_for_chunk(&mut chunk, address_offset)
.await
.map_err(|_| CfuProtocolError::WriterError(CfuWriterError::StorageError))?;
self.process_first_data_block(writer, chunk).await
}
num if (num < num_chunks) => {
image
.get_bytes_for_chunk(&mut chunk, address_offset)
.await
.map_err(|_| CfuProtocolError::WriterError(CfuWriterError::StorageError))?;
num if (num < num_chunks - 1) => {
self.process_middle_data_block(writer, chunk, i).await
}
_ => {
image
.get_bytes_for_chunk(
chunk
.get_mut(0..remainder)
.ok_or(CfuProtocolError::WriterError(CfuWriterError::Other))?,
address_offset,
)
.await
.map_err(|_| CfuProtocolError::WriterError(CfuWriterError::StorageError))?;
self.process_last_data_block(writer, chunk, i).await
_ /* num_chunks - 1 */ => {
if remainder == 0 {
self.process_last_data_block(writer, chunk, i).await
} else {
self.process_middle_data_block(writer, chunk, i).await
}
}
}
.map_err(CfuProtocolError::WriterError)?;

// if no errors in processing the data block, check the response
if r.status != CfuUpdateContentResponseStatus::Success {
return Err(CfuProtocolError::UpdateError(cmpt_id));
}
resp = r;
}

if remainder != 0 {
let mut last_chunk = [0u8; DEFAULT_DATA_LENGTH];
let address_offset = num_chunks * DEFAULT_DATA_LENGTH + base_offset;

image
.get_bytes_for_chunk(
last_chunk
.get_mut(0..remainder)
.ok_or(CfuProtocolError::WriterError(CfuWriterError::Other))?,
address_offset,
)
.await
.map_err(|_| CfuProtocolError::WriterError(CfuWriterError::StorageError))?;

let r = self
.process_last_data_block(writer, last_chunk, num_chunks)
.await
.map_err(CfuProtocolError::WriterError)?;
Comment on lines +160 to +163
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When processing the last chunk with a remainder (lines 138-162), the data_length field in the FwUpdateContentHeader will be incorrectly set to DEFAULT_DATA_LENGTH instead of the actual remainder size. The process_last_data_block function at line 234 always sets data_length to DEFAULT_DATA_LENGTH, but when called with a partial chunk, it should reflect the actual number of valid bytes (remainder). This could cause the device to process more bytes than actually provided or fail validation.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jerrysxie jerrysxie Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madeleyneVaca @Ctru14 @ankurung It seems like in process_last_data_block(), it is always assuming data length to be DEFAULT_DATA_LENGTH:

    async fn process_last_data_block(
        &mut self,
        w: &mut W,
        chunk: DataChunk,
        seq_num: usize,
    ) -> Result<FwUpdateContentResponse, CfuWriterError> {
        let cmd = FwUpdateContentCommand {
            header: FwUpdateContentHeader {
                flags: FW_UPDATE_FLAG_LAST_BLOCK,
                sequence_num: seq_num as u16,
                data_length: DEFAULT_DATA_LENGTH as u8, <----------------------------------------
                firmware_address: 0,
            },
            data: chunk,
        };
        let cmd_bytes: [u8; 60] = (&cmd).into();
        let offset = seq_num * DEFAULT_DATA_LENGTH;
        let mut resp_buf = [0u8; core::mem::size_of::<FwUpdateContentResponse>()];
        w.cfu_write_read(Some(offset), &cmd_bytes, &mut resp_buf)
            .await
            .map_err(|_| CfuWriterError::StorageError)?;

        FwUpdateContentResponse::try_from(resp_buf).map_err(|_| CfuWriterError::ByteConversionError)
    }

So even if there is fewer bytes, write a whole chunk?

Comment on lines +160 to +163
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the total image size is smaller than DEFAULT_DATA_LENGTH (num_chunks = 0, remainder > 0), the remainder chunk is processed as a last block without the first block flag. Since no iterations occur in the loop (0..0), the single chunk is sent only with the LAST_BLOCK flag at line 153, missing the required FIRST_BLOCK flag. For a single-chunk transfer, both flags should likely be set. Consider handling this edge case by checking if num_chunks == 0 and setting both flags accordingly.

Suggested change
let r = self
.process_last_data_block(writer, last_chunk, num_chunks)
.await
.map_err(CfuProtocolError::WriterError)?;
let r = if num_chunks == 0 {
// Single-chunk transfer: treat this remainder as the first (and only) block
self.process_first_data_block(writer, last_chunk).await
} else {
// There were full-sized chunks before; this is the final (last) block
self.process_last_data_block(writer, last_chunk, num_chunks).await
}
.map_err(CfuProtocolError::WriterError)?;

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jerrysxie jerrysxie Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madeleyneVaca @Ctru14 @ankurung Not sure what the correct expectation is here if an image is <= chunk length, marked it as both first and last? I will add check to enforce that there are at least 2 chunks worth of data right now.


// if no errors in processing the data block, check the response
if r.status != CfuUpdateContentResponseStatus::Success {
return Err(CfuProtocolError::UpdateError(cmpt_id));
}
resp = r;
}

let num_chunks = if remainder == 0 { num_chunks } else { num_chunks + 1 };
if resp.sequence != num_chunks as u16 {
trace!("final sequence number does not match expected number of chunks");
return Err(CfuProtocolError::InvalidBlockTransition);
Expand Down