Skip to content

fix: make GIF box map C2PA placeholder len 0 rather than 1#2156

Merged
ok-nick merged 6 commits into
mainfrom
ok-nick/gif-placeholder-length
Jun 1, 2026
Merged

fix: make GIF box map C2PA placeholder len 0 rather than 1#2156
ok-nick merged 6 commits into
mainfrom
ok-nick/gif-placeholder-length

Conversation

@ok-nick

@ok-nick ok-nick commented May 20, 2026

Copy link
Copy Markdown
Contributor

It can affect the box hashing code when generating hashes with minimal=true. This change also allows the code to be greatly simplified, I switched from declarative iterator to imperative-style for clarity.

This line here is removed:

/// ...
if should_insert_placeholder {
    offset += 1;
    /// ...

@ok-nick ok-nick requested a review from mauricefisher64 May 20, 2026 16:13
@codspeed-hq

codspeed-hq Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 30 untouched benchmarks
⏩ 64 skipped benchmarks1

Comparing ok-nick/gif-placeholder-length (330d433) with main (1e40b5c)

Open in CodSpeed

Footnotes

  1. 64 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@tmathern tmathern added the check-release Add this label to any PR to invoke a larger suite of tests. label May 23, 2026
@tmathern

tmathern commented May 23, 2026

Copy link
Copy Markdown
Contributor

(Merging in latest main fixes an unused dependency issue flagged by the CI/CD jobs)

@tmathern

tmathern commented May 27, 2026

Copy link
Copy Markdown
Contributor

@ok-nick It can affect the box hashing code when generating hashes with minimal=true.. Can you elaborate what the symptom/observed bug is in this case?

@ok-nick

ok-nick commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@tmathern When doing a minimal form box hash (everything before c2pa, the c2pa, and after c2pa), the old GIF code gave the empty placeholder C2PA box a length of 1. That would offset all of the subsequent boxes by 1, which is incorrect and would hash the wrong data.

See here:

c2pa-rs/sdk/src/assertions/box_hash.rs

Lines 260 to 358 in 2c0695e

if minimal_form {
let mut before_c2pa = BoxMap {
names: Vec::new(),
alg: Some(alg.to_string()),
hash: ByteBuf::from(vec![]),
excluded: None,
pad: ByteBuf::from(vec![]),
range_start: 0,
range_len: 0,
};
let mut c2pa_box = BoxMap {
names: Vec::new(),
alg: Some(alg.to_string()),
hash: ByteBuf::from(vec![]),
excluded: None,
pad: ByteBuf::from(vec![]),
range_start: 0,
range_len: 0,
};
let mut after_c2pa = BoxMap {
names: Vec::new(),
alg: Some(alg.to_string()),
hash: ByteBuf::from(vec![]),
excluded: None,
pad: ByteBuf::from(vec![]),
range_start: 0,
range_len: 0,
};
let mut is_before_c2pa = true;
// collapse map list to minimal set
for bm in source_bms.into_iter() {
if bm.names[0] == "C2PA" {
// there should only be 1 collapsed C2PA range
if bm.names.len() != 1 {
return Err(Error::HashMismatch("Malformed C2PA box hash".to_owned()));
}
c2pa_box = bm;
is_before_c2pa = false;
continue;
}
if is_before_c2pa {
before_c2pa.names.extend(bm.names);
if before_c2pa.range_len == 0 {
before_c2pa.range_start = bm.range_start;
before_c2pa.range_len = bm.range_len;
} else {
before_c2pa.range_len += bm.range_len;
}
} else {
after_c2pa.names.extend(bm.names);
if after_c2pa.range_len == 0 {
after_c2pa.range_start = bm.range_start;
after_c2pa.range_len = bm.range_len;
} else {
after_c2pa.range_len += bm.range_len;
}
}
}
// Instead of assuming we can combine all of the different ranges of
// box hashes, we will check the bounds of each one
let mut boxes = Vec::<BoxMap>::new();
// Only add if we have some before the C2PA box
if before_c2pa.range_len > 0 {
boxes.push(before_c2pa);
}
// Do the same for the actual C2PA box
if c2pa_box.range_len > 0 {
boxes.push(c2pa_box);
}
// And finally, add the boxes after the C2PA box
if after_c2pa.range_len > 0 {
boxes.push(after_c2pa);
}
self.boxes = boxes;
// compute the hashes
for bm in self.boxes.iter_mut() {
// skip c2pa box
if bm.names[0] == C2PA_BOXHASH {
continue;
}
let inclusions = vec![HashRange::new(bm.range_start, bm.range_len)];
bm.hash = ByteBuf::from(hash_stream_by_alg_with_progress(
alg,
reader,
Some(inclusions),
false,
&mut progress,
)?);
}
} else {

@ok-nick ok-nick merged commit bf3aa9c into main Jun 1, 2026
78 of 79 checks passed
@ok-nick ok-nick deleted the ok-nick/gif-placeholder-length branch June 1, 2026 19:30
This was referenced Jun 1, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpeacock gpeacock gpeacock approved these changes

@tmathern tmathern tmathern approved these changes

@mauricefisher64 mauricefisher64 Awaiting requested review from mauricefisher64

Assignees

No one assigned

Labels

check-release Add this label to any PR to invoke a larger suite of tests. safe to test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

GIF boxes hash placeholder is of length 1 when it should be 0

3 participants

@ok-nick @tmathern @gpeacock