Skip to content

Commit d4bf961

Browse files
Fix address bounds checking and metadata calculation for 64 KiB binaries
Prevent `u16` truncation when calculating end addresses or bounds for full 64 KiB binaries where `len as u16` wrapped 65536 to 0. - Simplify `AppState::is_external` to `addr.offset_from(self.origin) >= self.raw_data.len()`. - Fix `FileInfo` end address calculation for 64 KiB binaries to correctly report `$FFFF`. - Update analyzer routines (`promote_return_labels`, `follow_indirect_jumps`, `flow_analyze`) and HTML exporter to use canonical `AppState::is_external` checks. - Fix end address calculation in D64 and CRT picker dialogs. - Add test case verifying MCP address details on 64 KiB projects (#42). Fixes #42
1 parent 83855c9 commit d4bf961

6 files changed

Lines changed: 104 additions & 64 deletions

File tree

‎crates/regenerator2000-core/src/analyzer.rs‎

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -374,8 +374,6 @@ fn analyze_instruction(
374374
fn promote_return_labels(state: &AppState, usage_map: &mut BTreeMap<Addr, UsageData>) {
375375
let origin = state.origin;
376376
let data = &state.raw_data;
377-
let data_len = data.len();
378-
let end_addr = origin.wrapping_add(data_len as u16);
379377

380378
for (addr, (_types, _refs, first_type)) in usage_map.iter_mut() {
381379
// Only promote code-flow label types.
@@ -388,16 +386,11 @@ fn promote_return_labels(state: &AppState, usage_map: &mut BTreeMap<Addr, UsageD
388386
}
389387

390388
// Only for internal addresses (we cannot inspect external bytes).
391-
let is_internal = if origin < end_addr {
392-
addr.0 >= origin.0 && addr.0 < end_addr.0
393-
} else {
394-
addr.0 >= origin.0 || addr.0 < end_addr.0
395-
};
396-
if !is_internal {
389+
if state.is_external(*addr) {
397390
continue;
398391
}
399392

400-
let offset = addr.0.wrapping_sub(origin.0) as usize;
393+
let offset = addr.offset_from(origin);
401394

402395
// Target must be in a Code block.
403396
if state
@@ -452,7 +445,6 @@ fn follow_indirect_jumps(
452445
let data = &state.raw_data;
453446
let origin = state.origin;
454447
let data_len = data.len();
455-
let end_addr = origin.wrapping_add(data_len as u16);
456448

457449
let mut pc = 0;
458450
while pc < data_len {
@@ -491,17 +483,10 @@ fn follow_indirect_jumps(
491483
}
492484

493485
let jmp_addr = origin.wrapping_add(pc as u16);
494-
let pointer_addr: u16 = u16::from(data[pc + 2]) << 8 | u16::from(data[pc + 1]);
495-
496-
// Check if pointer_addr is inside our binary
497-
let is_internal = if origin < end_addr {
498-
pointer_addr >= origin.0 && pointer_addr < end_addr.0
499-
} else {
500-
pointer_addr >= origin.0 || pointer_addr < end_addr.0
501-
};
486+
let pointer_addr = Addr(u16::from(data[pc + 2]) << 8 | u16::from(data[pc + 1]));
502487

503-
if is_internal {
504-
let ptr_offset = pointer_addr.wrapping_sub(origin.0) as usize;
488+
if !state.is_external(pointer_addr) {
489+
let ptr_offset = pointer_addr.offset_from(origin);
505490

506491
// Check if the pointer location is an Address block
507492
let is_address_block = ptr_offset + 1 < data_len
@@ -552,19 +537,10 @@ pub fn flow_analyze(state: &AppState, start: Addr) -> Vec<std::ops::Range<usize>
552537

553538
let origin = state.origin;
554539
let data_len = state.raw_data.len();
555-
let end_addr = origin.wrapping_add(data_len as u16);
556540

557541
let to_offset = |addr: Addr| -> Option<usize> {
558-
let in_bounds = if origin < end_addr {
559-
addr.0 >= origin.0 && addr.0 < end_addr.0
560-
} else {
561-
addr.0 >= origin.0 || addr.0 < end_addr.0
562-
};
563-
if in_bounds {
564-
Some(addr.0.wrapping_sub(origin.0) as usize)
565-
} else {
566-
None
567-
}
542+
let offset = addr.offset_from(origin);
543+
(offset < data_len).then_some(offset)
568544
};
569545

570546
if let Some(offset) = to_offset(start) {

‎crates/regenerator2000-core/src/exporter/html.rs‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -257,8 +257,6 @@ pub fn export_html(state: &AppState, path: &PathBuf) -> Result<(), CoreError> {
257257
&& !refs.is_empty()
258258
&& state.settings.max_xref_count > 0
259259
{
260-
let origin_val = state.origin.0;
261-
let size = state.raw_data.len() as u16;
262260
let mut all_refs = refs.clone();
263261
all_refs.sort_unstable();
264262
all_refs.dedup();
@@ -268,7 +266,7 @@ pub fn export_html(state: &AppState, path: &PathBuf) -> Result<(), CoreError> {
268266
.iter()
269267
.take(max)
270268
.map(|r| {
271-
if r.0 >= origin_val && r.0 < origin_val + size {
269+
if !state.is_external(*r) {
272270
format!("<a href=\"#L{:04X}\">${:04X}</a>", r.0, r.0)
273271
} else {
274272
format!("${:04X}", r.0)
@@ -350,9 +348,7 @@ pub fn export_html(state: &AppState, path: &PathBuf) -> Result<(), CoreError> {
350348
// 6. Instruction part
351349
let mut operand_str = line.operand.clone();
352350
if let Some(target) = line.target_address {
353-
let origin_val = state.origin.0;
354-
let size = state.raw_data.len() as u16;
355-
if target.0 >= origin_val && target.0 < origin_val + size {
351+
if !state.is_external(target) {
356352
operand_str = format!("<a href=\"#L{:04X}\">{}</a>", target.0, line.operand);
357353
}
358354
} else {
@@ -364,10 +360,14 @@ pub fn export_html(state: &AppState, path: &PathBuf) -> Result<(), CoreError> {
364360
} else {
365361
if !current_word.is_empty() {
366362
if let Some(addr) = label_name_to_addr.get(&current_word) {
367-
new_op.push_str(&format!(
368-
"<a href=\"#L{:04X}\">{}</a>",
369-
addr.0, current_word
370-
));
363+
if !state.is_external(*addr) {
364+
new_op.push_str(&format!(
365+
"<a href=\"#L{:04X}\">{}</a>",
366+
addr.0, current_word
367+
));
368+
} else {
369+
new_op.push_str(&current_word);
370+
}
371371
} else {
372372
new_op.push_str(&current_word);
373373
}
@@ -378,10 +378,14 @@ pub fn export_html(state: &AppState, path: &PathBuf) -> Result<(), CoreError> {
378378
}
379379
if !current_word.is_empty() {
380380
if let Some(addr) = label_name_to_addr.get(&current_word) {
381-
new_op.push_str(&format!(
382-
"<a href=\"#L{:04X}\">{}</a>",
383-
addr.0, current_word
384-
));
381+
if !state.is_external(*addr) {
382+
new_op.push_str(&format!(
383+
"<a href=\"#L{:04X}\">{}</a>",
384+
addr.0, current_word
385+
));
386+
} else {
387+
new_op.push_str(&current_word);
388+
}
385389
} else {
386390
new_op.push_str(&current_word);
387391
}
@@ -400,16 +404,13 @@ pub fn export_html(state: &AppState, path: &PathBuf) -> Result<(), CoreError> {
400404
let mut comment_str = line.comment.clone();
401405
// Format x-refs in comments as hyperlinks if they are internal
402406
if comment_str.contains("x-ref:") {
403-
let origin_val = state.origin.0;
404-
let size = state.raw_data.len() as u16;
405407
let words: Vec<String> = comment_str
406408
.split(' ')
407409
.map(|w| {
408410
if w.starts_with('$') {
409411
let hex_str = w.trim_matches(|c| c == '$' || c == ',' || c == ';');
410412
if let Ok(addr) = u16::from_str_radix(hex_str, 16)
411-
&& addr >= origin_val
412-
&& addr < origin_val + size
413+
&& !state.is_external(crate::state::Addr(addr))
413414
{
414415
return format!("<a href=\"#L{:04X}\">${:04X}</a>", addr, addr)
415416
+ if w.ends_with(',') { "," } else { "" };

‎crates/regenerator2000-core/src/state/app_state.rs‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -262,16 +262,7 @@ impl AppState {
262262

263263
#[must_use]
264264
pub fn is_external(&self, addr: Addr) -> bool {
265-
let len = self.raw_data.len();
266-
if len == 0 {
267-
return true;
268-
}
269-
let end = self.origin.wrapping_add(len as u16);
270-
if self.origin < end {
271-
addr < self.origin || addr >= end
272-
} else {
273-
!(addr >= self.origin || addr < end)
274-
}
265+
addr.offset_from(self.origin) >= self.raw_data.len()
275266
}
276267

277268
#[must_use]
@@ -298,7 +289,7 @@ impl AppState {
298289
let end_addr = if size == 0 {
299290
start_addr
300291
} else {
301-
Addr(start_addr.0.saturating_add(size as u16).saturating_sub(1))
292+
start_addr.wrapping_add(size.saturating_sub(1) as u16)
302293
};
303294

304295
let mut mem = vec![0u8; 0x10000];
@@ -312,7 +303,10 @@ impl AppState {
312303
let packer = crate::packers::detect_packer(
313304
&mem,
314305
start_addr.0,
315-
load_start.saturating_add(size) as u16,
306+
(load_start
307+
.saturating_add(size)
308+
.min(0x10000)
309+
.saturating_sub(1)) as u16,
316310
);
317311

318312
let (is_packed, packer_name, entry_point) = if let Some(ref p) = packer {
@@ -615,4 +609,25 @@ mod tests {
615609
assert_eq!(info.entry_point, Some(Addr(2061)));
616610
assert!(!info.is_packed);
617611
}
612+
613+
#[test]
614+
fn test_file_info_full_64k() {
615+
let mut app_state = AppState::new();
616+
app_state.origin = Addr(0x0000);
617+
app_state.raw_data = vec![0xEA; 65536];
618+
let info = app_state.file_info();
619+
assert_eq!(info.start_addr, Addr(0x0000));
620+
assert_eq!(info.end_addr, Addr(0xFFFF));
621+
assert_eq!(info.size, 65536);
622+
}
623+
624+
#[test]
625+
fn test_is_external_full_64k() {
626+
let mut app_state = AppState::new();
627+
app_state.origin = Addr(0x0000);
628+
app_state.raw_data = vec![0xEA; 65536];
629+
assert!(!app_state.is_external(Addr(0x0000)));
630+
assert!(!app_state.is_external(Addr(0x0810)));
631+
assert!(!app_state.is_external(Addr(0xFFFF)));
632+
}
618633
}

‎crates/regenerator2000-tui/src/ui/dialog_crt_picker.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ impl Widget for CrtBankPickerDialog {
8888
.map(|picker_entry| {
8989
let chip = &picker_entry.chip;
9090
let start = chip.load_address;
91-
let end = start.wrapping_add(chip.data.len() as u16).wrapping_sub(1);
91+
let end = start.wrapping_add(chip.data.len().saturating_sub(1) as u16);
9292

9393
let addr_str = format!("${start:04X}-${end:04X}");
9494
let entropy_str = format!("{:>5.2}", picker_entry.entropy);

‎crates/regenerator2000-tui/src/ui/dialog_d64_picker.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,7 @@ impl D64FilePickerDialog {
5353
end_addr = Some(
5454
prg_data
5555
.origin
56-
.wrapping_add(prg_data.raw_data.len() as u16)
57-
.wrapping_sub(1),
56+
.wrapping_add(prg_data.raw_data.len().saturating_sub(1) as u16),
5857
);
5958
entropy = Some(crate::utils::calculate_entropy(&prg_data.raw_data));
6059
}

‎tests/mcp_address_details_test.rs‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,4 +153,53 @@ mod tests {
153153
assert!(refs.iter().any(|v| v.as_u64() == Some(0x1005)));
154154
}
155155
}
156+
157+
#[test]
158+
fn test_address_details_full_64k() {
159+
let mut app_state = AppState::default();
160+
app_state.system_config.default_is_unexplored = false;
161+
let origin = regenerator2000_core::state::Addr(0x0000);
162+
let mut data = vec![0u8; 65536];
163+
// At $0810 (2064):
164+
// LDA #$1B (A9 1B)
165+
// STA $D011 (8D 11 D0)
166+
// LDA #$00 (A9 00)
167+
// STA $D020 (8D 20 D0)
168+
// JSR $FFD2 (20 D2 FF)
169+
// RTS (60)
170+
data[0x0810..0x081E].copy_from_slice(&[
171+
0xA9, 0x1B, 0x8D, 0x11, 0xD0, 0xA9, 0x00, 0x8D, 0x20, 0xD0, 0x20, 0xD2, 0xFF, 0x60,
172+
]);
173+
app_state.load_binary(origin, data).unwrap();
174+
175+
let mut ui_state = UIState::new(Theme::default());
176+
177+
let details = call_tool(
178+
&mut app_state,
179+
&mut ui_state,
180+
"r2000_get_address_details",
181+
json!({ "address": 2064 }),
182+
);
183+
184+
assert_eq!(details["address"], 2064);
185+
assert_eq!(details["type"], "Code");
186+
assert_eq!(details["instruction"]["mnemonic"], "LDA");
187+
188+
// First address ($0000) and last address ($FFFF) should also be valid details
189+
let details_first = call_tool(
190+
&mut app_state,
191+
&mut ui_state,
192+
"r2000_get_address_details",
193+
json!({ "address": 0 }),
194+
);
195+
assert_eq!(details_first["address"], 0);
196+
197+
let details_last = call_tool(
198+
&mut app_state,
199+
&mut ui_state,
200+
"r2000_get_address_details",
201+
json!({ "address": 0xFFFF }),
202+
);
203+
assert_eq!(details_last["address"], 0xFFFF);
204+
}
156205
}

0 commit comments

Comments
 (0)