Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

properly initialize radix #528

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DennyDai
Copy link

@DennyDai DennyDai commented May 6, 2022

AsmLexer.cpp#L28

AsmLexer::AsmLexer(const MCAsmInfo &MAI) : MAI(MAI) {
  CurPtr = nullptr;
  isAtStartOfLine = true;
  AllowAtInIdentifier = !StringRef(MAI.getCommentString()).startswith("@");
  defaultRadix = MAI.getRadix(); // <----
}

The dafaultRadix field is set to MAI.getRadix(), and MAI.Radix may not be set at the time MAI.getRadix() is called.
PR #382 attempted to fix this issue by adding ks->MAI->setRadix(16) to ks_option(), but ks_option() may not be called when assembling the instructions, for example, on keystone's offcial tutorial

ks = Ks(KS_ARCH_X86, KS_MODE_32)
encoding, count = ks.asm(CODE)

In this case MAI.Radix is still not initialized, so defaultRadix can be any value. When defaultRadix is anything other than 16, then [1-9][0-9]* in instructions will be assembled as decimal values (AsmLexer.cpp#L262), otherwise when defaultRadix is 16, [1-9][0-9]* in instructions will be assembled as hex values (AsmLexer.cpp#L264). As a result, the behavior will be unpredictable.


I think the default radix should be 10 because otherwise we won't be able to assemble instructions with decimal values, If users wish to change the default radix to 16, they can use KS_OPT_SYNTAX_RADIX16 option.
AsmLexer.cpp#L265

AsmToken AsmLexer::LexDigit()
{
  // Decimal integer: [1-9][0-9]*
  if (CurPtr[-1] != '0' || CurPtr[0] == '.') {
    unsigned Radix = doLookAhead(CurPtr, 10);

    if (defaultRadix == 16)
      Radix = 16;
......

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant