From fc223eb5936cc22c74764e528cbd99490d38d212 Mon Sep 17 00:00:00 2001 From: Alex Kiernan Date: Fri, 31 Jul 2026 13:18:42 +0100 Subject: [PATCH] pack: stop prepending a space to model, manufacturer and id pack_rkaf prepends a leading space to three fixed-width identity fields of the RKAF header: let model_str = if model.starts_with(' ') { model.to_string() } else { format!(" {}", model) }; and the same for manufacturer, and unconditionally for id. That looks like it is imitating genuine Rockchip images, which really do carry a leading space on the manufacturer field - but not because the vendor packer put one there. Comparing three RK3308 factory update images against the parameter.txt each one carries, field by field: parameter.txt RKAF header MACHINE_MODEL:RK3308 model='RK3308' MACHINE_ID:007 id='007' MANUFACTURER: RK3308 manufacturer=' RK3308' All three images, identical. The header holds each value exactly as the parameter file spelled it, space and all. rkpack.c writing the fields with a plain "%s", and rkflashtool's rkunpack.c reading the model straight from offset 0x08, are the same story from the other side. The vendor tool prepends nothing: the space belongs to one line of the vendor's parameter.txt, MANUFACTURER being the only one of the three the Rockchip parameter-file spec's own example writes spaced. So this code matched the reference output on manufacturer by accident and diverged from it on the other two - backwards from following it. id is the clearest case: the value is read from parameter.txt and .trim()ed, discarding whatever spacing the file used, then a space is synthesised back on. Copying verbatim is both simpler and what the vendor does. These are machine-readable fields. A consumer comparing model against a known board identity gets a mismatch for a reason that has nothing to do with the image, and its only ways out are to strip whitespace from a fixed-width binary field or to hard-code an offset of 9. It also spends a byte of a bounded field that write_cstr_preserving_tail already has to share with the NUL. Nothing is taken away from callers that want the vendor's spacing: model and manufacturer are supplied by the caller and are now written through untouched, so passing " RK3308" still yields " RK3308". Where the leading space goes is the caller's decision, which is the point - it should be a decision, not something the packer does behind them. Two existing tests asserted the space (" RK3588\0", " 007\0") - their expectations are updated. Neither was testing the padding: one covers string-field tail preservation across a repack, the other that a removed MACHINE_ID does not leak from a saved template, and both still cover exactly that. A new test pins the fixed behaviour, checking all three fields land flush and that bytes 8, 42 and 72 are not spaces. Signed-off-by: Alex Kiernan --- src/pack.rs | 17 +++--------- tests/bugfix_tests.rs | 61 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 17 deletions(-) diff --git a/src/pack.rs b/src/pack.rs index f64807c..b1ea9c2 100644 --- a/src/pack.rs +++ b/src/pack.rs @@ -460,22 +460,11 @@ pub fn pack_rkaf(input_dir: &str, output_file: &str, model: &str, manufacturer: }; header.magic.copy_from_slice(RKAF_SIGNATURE); - let model_str = if model.starts_with(' ') { - model.to_string() - } else { - format!(" {}", model) - }; - write_cstr_preserving_tail(&mut header.model, &model_str); - - let manufacturer_str = if manufacturer.starts_with(' ') { - manufacturer.to_string() - } else { - format!(" {}", manufacturer) - }; - write_cstr_preserving_tail(&mut header.manufacturer, &manufacturer_str); + write_cstr_preserving_tail(&mut header.model, model); + write_cstr_preserving_tail(&mut header.manufacturer, manufacturer); if !machine_id.is_empty() { - write_cstr_preserving_tail(&mut header.id, &format!(" {}", machine_id)); + write_cstr_preserving_tail(&mut header.id, &machine_id); } else { // No MACHINE_ID in parameter.txt: clear the logical id so a template // value cannot leak through. Bytes after the NUL are left untouched diff --git a/tests/bugfix_tests.rs b/tests/bugfix_tests.rs index 91d2d4d..c76523f 100644 --- a/tests/bugfix_tests.rs +++ b/tests/bugfix_tests.rs @@ -277,6 +277,61 @@ mod tests { ); } + /// model, id and manufacturer are fixed-width machine-readable fields that + /// the vendor tool copies verbatim out of parameter.txt: rkpack.c writes + /// them with a plain "%s" and rkflashtool's rkunpack.c reads the model + /// straight from offset 0x08. Genuine RK3308 factory images bear that out - + /// model and id sit flush at bytes 8 and 42, and the one field that does + /// carry a leading space (manufacturer) carries it in their parameter.txt + /// too. So pack must prepend nothing of its own, or a consumer comparing a + /// field against a known board identity fails for a reason that has nothing + /// to do with the image. + #[test] + fn rkaf_identity_fields_have_no_leading_space() { + let dir = TempDir::new().unwrap(); + let input = dir.path(); + fs::write(input.join("parameter.txt"), b"MACHINE_ID:007\nCMDLINE:x\n").unwrap(); + fs::write(input.join("userdata.img"), vec![0xAAu8; 100]).unwrap(); + fs::write( + input.join("package-file"), + "parameter parameter.txt\nuserdata userdata.img\n", + ) + .unwrap(); + fs::write( + input.join("partition-metadata.txt"), + "parameter,parameter.txt,0x00004000,0x00000000,0x00000800,0x00000001,0x00000016\n\ + userdata,userdata.img,0xffffffff,0x00a98000,0x00001000,0x00000001,0x00000064\n", + ) + .unwrap(); + + let out = dir.path().join("out.rkaf"); + pack_rkaf( + input.to_str().unwrap(), + out.to_str().unwrap(), + "rithum,switch-pro", + "Rithum", + ) + .unwrap(); + + let img = fs::read(&out).unwrap(); + + // magic[4] + length[4] => model[34] at 8, id[30] at 42, + // manufacturer[56] at 72. + let field = |off: usize, len: usize| { + let raw = &img[off..off + len]; + let end = raw.iter().position(|&b| b == 0).unwrap_or(len); + std::str::from_utf8(&raw[..end]).unwrap().to_string() + }; + + assert_eq!(field(8, 34), "rithum,switch-pro", "model must start at byte 8"); + assert_eq!(field(42, 30), "007", "id must not be padded"); + assert_eq!(field(72, 56), "Rithum", "manufacturer must not be padded"); + + assert_ne!(img[8], b' ', "model must not be space-prefixed"); + assert_ne!(img[42], b' ', "id must not be space-prefixed"); + assert_ne!(img[72], b' ', "manufacturer must not be space-prefixed"); + } + /// The vendor tool leaves undocumented bytes (e.g. 0x48 0x01) in the tails /// of string fields. unpack must save the original RKAF header and pack /// must use it as a template so those bytes survive a round-trip. @@ -332,7 +387,7 @@ mod tests { assert_eq!(img[0x26], 0x01, "model-field tail byte must survive repack"); assert_eq!(img[part1_path_tail], 0x48, "path-field tail byte must survive repack"); // sanity: the real strings are still intact and NUL-terminated - assert_eq!(&img[8..16], b" RK3588\0"); + assert_eq!(&img[8..15], b"RK3588\0"); assert_eq!(&img[140 + 112..140 + 112 + 9], b"userdata\0"); } @@ -363,8 +418,8 @@ mod tests { let img1 = dir.path().join("v1.rkaf"); pack_rkaf(input.to_str().unwrap(), img1.to_str().unwrap(), "RK3588", "RK3588").unwrap(); assert_eq!( - &fs::read(&img1).unwrap()[ID_OFFSET..ID_OFFSET + 5], - b" 007\0", + &fs::read(&img1).unwrap()[ID_OFFSET..ID_OFFSET + 4], + b"007\0", "id must be set while MACHINE_ID exists" );