fix: accept hexadecimal values for Disassemble x86 address arguments - #2721
Open
arjun2075 wants to merge 1 commit into
Open
fix: accept hexadecimal values for Disassemble x86 address arguments#2721arjun2075 wants to merge 1 commit into
arjun2075 wants to merge 1 commit into
Conversation
The 'Code Segment (CS)' and 'Offset (IP)' arguments were declared as
'number' ingredients, so they were parsed with parseFloat and rejected
hexadecimal input such as '0xABC' with:
Invalid ingredient value. Not a number: NaN
However, SetBasePosition already parses both values with parseInt(x, 16)
and GetPosition renders them back out with toString(16), so these fields
have always been hexadecimal. The two conversions cancel out visually,
which is why the mismatch went unnoticed. It is observable though: a code
segment entered as '24' becomes 36 decimal internally, which crosses the
documented 'CS >= 36 switches 32-bit output to SEG:OFFSET' threshold.
Both arguments are now 'string' ingredients parsed by a dedicated helper
that accepts bare ('ABC'), prefixed ('0xABC') and suffixed ('ABCh') hex,
and rejects anything else with a named error instead of a NaN message.
The code segment is left-padded to four digits because SetBasePosition
reads it via slice(length - 4), which silently truncated shorter values
('ABC' was read as 'C').
Existing recipes are unaffected: the default is unchanged and numeric
argument values from saved recipes are still handled.
Fixes gchq#2720
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
Code Segment (CS)andOffset (IP)arguments of the Disassemble x86 operation were declared asnumberingredients.Ingredient.prepareparses those withparseFloat, so any hexadecimal input was rejected before reaching the operation:The underlying cause runs deeper than the reported symptom.
SetBasePositionalready parses both values withparseInt(x, 16), andGetPositionrenders them back out withtoString(16)— these fields have always been hexadecimal. The hex-parse on input and hex-format on output cancel each other visually, which is why the mismatch went unnoticed for so long.It is observable, though. A code segment entered as
24becomes36decimal internally, which crosses the documented "CS >= 36 switches 32-bit output to SEG:OFFSET" threshold, while35becomes53.Changes:
stringingredients, parsed by a smallparseHexAddresshelper that accepts the three common ways of writing a hex literal — bare (ABC), prefixed (0xABC) and suffixed (ABCh) — and rejects anything else with a named error instead of aNaNmessage.SetBasePositionreads it viaslice(length - 4)and silently truncated shorter values (ABCwas read asC, i.e. 12). Without this, the exact input from the issue would still have produced a wrong result.Offset (IP)is fixed alongsideCS, as it is hex-parsed by the same function and had the identical latent bug.Existing recipes are unaffected: the default value is unchanged, and numeric argument values from previously saved recipes are still handled (verified explicitly).
Existing Issue
Fixes #2720
Screenshots
N/A — no layout or styling changes. The
Code Segment (CS)andOffset (IP)arguments change from number inputs to text inputs with a hex format hint, which follows directly from the ingredient type change in the diff.AI disclosure
This change was written with AI assistance using Claude (Claude Code, model Opus 5). The root cause analysis, the hex/decimal mismatch, the
slice(length - 4)truncation and the test cases were all verified by executing the code rather than by inspection alone. I have reviewed the change and understand it.Test Coverage
Added
tests/operations/tests/DisassembleX86.mjs— the operation previously had no test coverage (only Disassemble ARM did). Seven tests cover:0xABC,ABCh,ABC0x1000)Full suite results locally:
tests/operations— 2287/2287 passingtests/node— 272/272 passingnpx grunt lint— clean