Skip to content

Commit 976e629

Browse files
LagoLunaticLagoLunatic
authored
Fix non-strings also being escaped when copied (#307)
* Fix non-strings also being escaped when copied * Move copy escape logic into objdiff-core
1 parent 67b28b7 commit 976e629

File tree

6 files changed

+75
-49
lines changed

6 files changed

+75
-49
lines changed

‎objdiff-core/src/arch/mod.rs‎

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ impl fmt::Display for DataType {
8383
impl DataType {
8484
pub fn display_labels(&self, endian: object::Endianness, bytes: &[u8]) -> Vec<String> {
8585
let mut strs = Vec::new();
86-
for (literal, label_override) in self.display_literals(endian, bytes) {
86+
for (literal, label_override, _) in self.display_literals(endian, bytes) {
8787
let label = label_override.unwrap_or_else(|| self.to_string());
8888
strs.push(format!("{label}: {literal:?}"))
8989
}
@@ -94,7 +94,7 @@ impl DataType {
9494
&self,
9595
endian: object::Endianness,
9696
bytes: &[u8],
97-
) -> Vec<(String, Option<String>)> {
97+
) -> Vec<(String, Option<String>, Option<String>)> {
9898
let mut strs = Vec::new();
9999
if self.required_len().is_some_and(|l| bytes.len() < l) {
100100
log::warn!(
@@ -118,34 +118,34 @@ impl DataType {
118118
match self {
119119
DataType::Int8 => {
120120
let i = i8::from_ne_bytes(bytes.try_into().unwrap());
121-
strs.push((format!("{i:#x}"), None));
121+
strs.push((format!("{i:#x}"), None, None));
122122

123123
if i < 0 {
124-
strs.push((format!("{:#x}", ReallySigned(i)), None));
124+
strs.push((format!("{:#x}", ReallySigned(i)), None, None));
125125
}
126126
}
127127
DataType::Int16 => {
128128
let i = endian.read_i16_bytes(bytes.try_into().unwrap());
129-
strs.push((format!("{i:#x}"), None));
129+
strs.push((format!("{i:#x}"), None, None));
130130

131131
if i < 0 {
132-
strs.push((format!("{:#x}", ReallySigned(i)), None));
132+
strs.push((format!("{:#x}", ReallySigned(i)), None, None));
133133
}
134134
}
135135
DataType::Int32 => {
136136
let i = endian.read_i32_bytes(bytes.try_into().unwrap());
137-
strs.push((format!("{i:#x}"), None));
137+
strs.push((format!("{i:#x}"), None, None));
138138

139139
if i < 0 {
140-
strs.push((format!("{:#x}", ReallySigned(i)), None));
140+
strs.push((format!("{:#x}", ReallySigned(i)), None, None));
141141
}
142142
}
143143
DataType::Int64 => {
144144
let i = endian.read_i64_bytes(bytes.try_into().unwrap());
145-
strs.push((format!("{i:#x}"), None));
145+
strs.push((format!("{i:#x}"), None, None));
146146

147147
if i < 0 {
148-
strs.push((format!("{:#x}", ReallySigned(i)), None));
148+
strs.push((format!("{:#x}", ReallySigned(i)), None, None));
149149
}
150150
}
151151
DataType::Float => {
@@ -156,6 +156,7 @@ impl DataType {
156156
object::Endianness::Big => f32::from_be_bytes(bytes),
157157
}),
158158
None,
159+
None,
159160
));
160161
}
161162
DataType::Double => {
@@ -166,24 +167,29 @@ impl DataType {
166167
object::Endianness::Big => f64::from_be_bytes(bytes),
167168
}),
168169
None,
170+
None,
169171
));
170172
}
171173
DataType::Bytes => {
172-
strs.push((format!("{bytes:#?}"), None));
174+
strs.push((format!("{bytes:#?}"), None, None));
173175
}
174176
DataType::String => {
175177
if let Some(nul_idx) = bytes.iter().position(|&c| c == b'\0') {
176178
let str_bytes = &bytes[..nul_idx];
177179
// Special case to display (ASCII) as the label for ASCII-only strings.
178180
let (cow, _, had_errors) = encoding_rs::UTF_8.decode(str_bytes);
179181
if !had_errors && cow.is_ascii() {
180-
strs.push((format!("{cow}"), Some("ASCII".into())));
182+
let string = format!("{cow}");
183+
let copy_string = escape_special_ascii_characters(string.clone());
184+
strs.push((string, Some("ASCII".into()), Some(copy_string)));
181185
}
182186
for (encoding, encoding_name) in SUPPORTED_ENCODINGS {
183187
let (cow, _, had_errors) = encoding.decode(str_bytes);
184188
// Avoid showing ASCII-only strings more than once if the encoding is ASCII-compatible.
185189
if !had_errors && (!encoding.is_ascii_compatible() || !cow.is_ascii()) {
186-
strs.push((format!("{cow}"), Some(encoding_name.into())));
190+
let string = format!("{cow}");
191+
let copy_string = escape_special_ascii_characters(string.clone());
192+
strs.push((string, Some(encoding_name.into()), Some(copy_string)));
187193
}
188194
}
189195
}
@@ -499,3 +505,21 @@ pub struct RelocationOverride {
499505
pub target: RelocationOverrideTarget,
500506
pub addend: i64,
501507
}
508+
509+
/// Escape ASCII characters such as \n or \t, but not Unicode characters such as \u{3000}.
510+
/// Suitable for copying to clipboard.
511+
fn escape_special_ascii_characters(value: String) -> String {
512+
let mut escaped = String::new();
513+
escaped.push('"');
514+
for c in value.chars() {
515+
if c.is_ascii() {
516+
for e in c.escape_default() {
517+
escaped.push(e);
518+
}
519+
} else {
520+
escaped.push(c);
521+
}
522+
}
523+
escaped.push('"');
524+
escaped
525+
}

‎objdiff-core/src/arch/ppc/mod.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -440,9 +440,13 @@ impl Arch for ArchPpc {
440440
let simplified = ins.simplified().to_string();
441441
let show_orig = orig != simplified;
442442
let mut out = Vec::with_capacity(2);
443-
out.push(ContextItem::Copy { value: simplified, label: None });
443+
out.push(ContextItem::Copy { value: simplified, label: None, copy_string: None });
444444
if show_orig {
445-
out.push(ContextItem::Copy { value: orig, label: Some("original".to_string()) });
445+
out.push(ContextItem::Copy {
446+
value: orig,
447+
label: Some("original".to_string()),
448+
copy_string: None,
449+
});
446450
}
447451
out
448452
}

‎objdiff-core/src/diff/display.rs‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ impl From<&DiffText<'_>> for HighlightKind {
366366
}
367367

368368
pub enum ContextItem {
369-
Copy { value: String, label: Option<String> },
369+
Copy { value: String, label: Option<String>, copy_string: Option<String> },
370370
Navigate { label: String, symbol_index: usize, kind: SymbolNavigationKind },
371371
Separator,
372372
}
@@ -398,16 +398,17 @@ pub fn symbol_context(obj: &Object, symbol_index: usize) -> Vec<ContextItem> {
398398
return Vec::new();
399399
};
400400
let mut out = Vec::new();
401-
out.push(ContextItem::Copy { value: symbol.name.clone(), label: None });
401+
out.push(ContextItem::Copy { value: symbol.name.clone(), label: None, copy_string: None });
402402
if let Some(name) = &symbol.demangled_name {
403-
out.push(ContextItem::Copy { value: name.clone(), label: None });
403+
out.push(ContextItem::Copy { value: name.clone(), label: None, copy_string: None });
404404
}
405405
if symbol.section.is_some()
406406
&& let Some(address) = symbol.virtual_address
407407
{
408408
out.push(ContextItem::Copy {
409409
value: format!("{address:x}"),
410410
label: Some("virtual address".to_string()),
411+
copy_string: None,
411412
});
412413
}
413414
out.append(&mut obj.arch.symbol_context(obj, symbol_index));
@@ -501,8 +502,8 @@ pub fn relocation_context(
501502
let literals = display_ins_data_literals(obj, ins);
502503
if !literals.is_empty() {
503504
out.push(ContextItem::Separator);
504-
for (literal, label_override) in literals {
505-
out.push(ContextItem::Copy { value: literal, label: label_override });
505+
for (literal, label_override, copy_string) in literals {
506+
out.push(ContextItem::Copy { value: literal, label: label_override, copy_string });
506507
}
507508
}
508509
}
@@ -598,24 +599,37 @@ pub fn instruction_context(
598599
for byte in resolved.code {
599600
hex_string.push_str(&format!("{byte:02x}"));
600601
}
601-
out.push(ContextItem::Copy { value: hex_string, label: Some("instruction bytes".to_string()) });
602+
out.push(ContextItem::Copy {
603+
value: hex_string,
604+
label: Some("instruction bytes".to_string()),
605+
copy_string: None,
606+
});
602607
out.append(&mut obj.arch.instruction_context(obj, resolved));
603608
if let Some(virtual_address) = resolved.symbol.virtual_address {
604609
let offset = resolved.ins_ref.address - resolved.symbol.address;
605610
out.push(ContextItem::Copy {
606611
value: format!("{:x}", virtual_address + offset),
607612
label: Some("virtual address".to_string()),
613+
copy_string: None,
608614
});
609615
}
610616
for arg in &ins.args {
611617
if let InstructionArg::Value(arg) = arg {
612-
out.push(ContextItem::Copy { value: arg.to_string(), label: None });
618+
out.push(ContextItem::Copy { value: arg.to_string(), label: None, copy_string: None });
613619
match arg {
614620
InstructionArgValue::Signed(v) => {
615-
out.push(ContextItem::Copy { value: v.to_string(), label: None });
621+
out.push(ContextItem::Copy {
622+
value: v.to_string(),
623+
label: None,
624+
copy_string: None,
625+
});
616626
}
617627
InstructionArgValue::Unsigned(v) => {
618-
out.push(ContextItem::Copy { value: v.to_string(), label: None });
628+
out.push(ContextItem::Copy {
629+
value: v.to_string(),
630+
label: None,
631+
copy_string: None,
632+
});
619633
}
620634
_ => {}
621635
}
@@ -677,7 +691,7 @@ pub fn instruction_hover(
677691
let literals = display_ins_data_literals(obj, resolved);
678692
if !literals.is_empty() {
679693
out.push(HoverItem::Separator);
680-
for (literal, label_override) in literals {
694+
for (literal, label_override, _) in literals {
681695
out.push(HoverItem::Text {
682696
label: label_override.unwrap_or_else(|| ty.to_string()),
683697
value: format!("{literal:?}"),
@@ -871,7 +885,7 @@ pub fn display_ins_data_labels(obj: &Object, resolved: ResolvedInstructionRef) -
871885
pub fn display_ins_data_literals(
872886
obj: &Object,
873887
resolved: ResolvedInstructionRef,
874-
) -> Vec<(String, Option<String>)> {
888+
) -> Vec<(String, Option<String>, Option<String>)> {
875889
let Some(reloc) = resolved.relocation else {
876890
return Vec::new();
877891
};

‎objdiff-gui/src/views/diff.rs‎

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -865,24 +865,6 @@ pub fn hover_items_ui(ui: &mut Ui, items: Vec<HoverItem>, appearance: &Appearanc
865865
}
866866
}
867867

868-
/// Escape ASCII characters such as \n or \t, but not Unicode characters such as \u{3000}.
869-
/// Suitable for copying to clipboard.
870-
fn escape_special_ascii_characters(value: String) -> String {
871-
let mut escaped = String::new();
872-
escaped.push('"');
873-
for c in value.chars() {
874-
if c.is_ascii() {
875-
for e in c.escape_default() {
876-
escaped.push(e);
877-
}
878-
} else {
879-
escaped.push(c);
880-
}
881-
}
882-
escaped.push('"');
883-
escaped
884-
}
885-
886868
pub fn context_menu_items_ui(
887869
ui: &mut Ui,
888870
items: Vec<ContextItem>,
@@ -892,7 +874,7 @@ pub fn context_menu_items_ui(
892874
let mut ret = None;
893875
for item in items {
894876
match item {
895-
ContextItem::Copy { value, label } => {
877+
ContextItem::Copy { value, label, copy_string } => {
896878
let mut job = LayoutJob::default();
897879
write_text("Copy ", appearance.text_color, &mut job, appearance.code_font.clone());
898880
write_text(
@@ -912,8 +894,7 @@ pub fn context_menu_items_ui(
912894
write_text(")", appearance.text_color, &mut job, appearance.code_font.clone());
913895
}
914896
if ui.button(job).clicked() {
915-
let escaped = escape_special_ascii_characters(value);
916-
ui.ctx().copy_text(escaped);
897+
ui.ctx().copy_text(copy_string.unwrap_or(value));
917898
ui.close();
918899
}
919900
}

‎objdiff-wasm/src/api.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,7 @@ impl GuestDisplay for Component {
273273
return vec![ContextItem::Copy(ContextItemCopy {
274274
value: "Failed to resolve instruction".to_string(),
275275
label: Some("error".to_string()),
276+
copy_string: None,
276277
})];
277278
};
278279
let ins = match obj.arch.process_instruction(resolved, &diff_config) {
@@ -281,6 +282,7 @@ impl GuestDisplay for Component {
281282
return vec![ContextItem::Copy(ContextItemCopy {
282283
value: e.to_string(),
283284
label: Some("error".to_string()),
285+
copy_string: None,
284286
})];
285287
}
286288
};
@@ -707,8 +709,8 @@ impl From<diff::display::HoverItemColor> for HoverItemColor {
707709
impl From<diff::display::ContextItem> for ContextItem {
708710
fn from(item: diff::display::ContextItem) -> Self {
709711
match item {
710-
diff::display::ContextItem::Copy { value, label } => {
711-
ContextItem::Copy(ContextItemCopy { value, label })
712+
diff::display::ContextItem::Copy { value, label, copy_string } => {
713+
ContextItem::Copy(ContextItemCopy { value, label, copy_string })
712714
}
713715
diff::display::ContextItem::Navigate { label, symbol_index, kind } => {
714716
ContextItem::Navigate(ContextItemNavigate {

‎objdiff-wasm/wit/objdiff.wit‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ interface display {
143143
record context-item-copy {
144144
value: string,
145145
label: option<string>,
146+
copy-string: option<string>,
146147
}
147148

148149
record context-item-navigate {

0 commit comments

Comments
 (0)