Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 14, 2025

Removed the sortParameterAnnotations function from tests and fixed both the DEX and Smali parsers to output parameter annotations in the correct order.

Investigation Findings

Original Hypothesis (incorrect):

  • DEX parser reads parameter annotations sequentially from annotationsDirectoryItem.parameterAnnotations
  • Smali parser processes methods sequentially as they appear in the file
  • Baksmali outputs methods in the same order as DEX method list
  • Therefore, parameter annotation order would naturally match

Actual Behavior:
Baksmali generates smali files with methods in its own ordering, which differs from the order in the DEX file's annotations directory. Additionally, DEX files may not always have parameter annotations in strictly sorted order.

Example from android/os/LocaleList before fix:

  • DEX order: <init>, equals, forLanguageTags
  • Smali order: <init>, forLanguageTags, equals

Root Cause

Neither parser was enforcing consistent ordering of parameter annotations. The correct order according to the DEX file format specification is: parameter annotations sorted by method index in the class's method list (directMethods + virtualMethods).

Solution

Fixed both parsers to ensure parameter annotations are always sorted by method index:

  1. Smali Parser (src/smaliParser.ts):

    • Added sorting after collecting parameter annotations
    • Sorts by method position in the combined directMethods + virtualMethods array
  2. DEX Parser (src/dalvikExecutableParser.ts):

    • Added sorting after all class definitions are created
    • Sorts each class's parameter annotations by method position in classData
    • Handles cases where DEX files don't follow strict ordering requirements
  3. Test File (src/dalvikExecutableParserAgainstSmaliParser.test.ts):

    • Removed sortParameterAnnotations function (no longer needed)
    • Tests now verify that both parsers produce the same correct order

Result

Both parsers now output parameter annotations in consistent order:

  • Sorted by method index in directMethods + virtualMethods
  • Matches DEX file format specification
  • No manual sorting required in tests

Example from android/os/LocaleList after fix (both parsers):

  • Order: <init> (index 1), forLanguageTags (index 2), equals (index 4)

Note: sortFieldAnnotations is retained as field annotations still require ordering normalization.

Original prompt

This section details on the original issue you should resolve

<issue_title>Remove sortParameterAnnotations only, check if smali code has the same order of parameter annotations that dex file has, check if our smali parser has the same order of annotations, if not, fix our smali parser</issue_title>
<issue_description></issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…already in correct order

Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove sortParameterAnnotations and validate annotations order Remove sortParameterAnnotations - parameter annotations already maintain DEX order Nov 14, 2025
Copilot AI requested a review from futpib November 14, 2025 20:20
Copilot finished work on behalf of futpib November 14, 2025 20:20
@futpib
Copy link
Owner

futpib commented Nov 14, 2025

@copilot this test fails dalvikExecutableParserAgainstSmaliParser › parse(dex) against parse(smali(dex)) bafybeiebe27ylo53trgitu6fqfbmba43c4ivxj3nt4kumsilkucpbdxtqq android/os/LocaleList isolated

parameterAnnotations: [
          Object { … },
          {
            annotations: Array [ … ],
            method: {
              class: 'Landroid/os/LocaleList;',
  -           name: 'equals',
  +           name: 'forLanguageTags',
              prototype: {
                parameters: [
  -               'Ljava/lang/Object;',
  +               'Ljava/lang/String;',
                ],
  -             returnType: 'Z',
  +             returnType: 'Landroid/os/LocaleList;',
  -             shorty: 'ZL',
  +             shorty: 'LL',
              },
            },
          },
          {
            annotations: Array [ … ],
            method: {
              class: 'Landroid/os/LocaleList;',
  -           name: 'forLanguageTags',
  +           name: 'equals',
              prototype: {
                parameters: [
  -               'Ljava/lang/String;',
  +               'Ljava/lang/Object;',
                ],
  -             returnType: 'Landroid/os/LocaleList;',
  +             returnType: 'Z',
  -             shorty: 'LL',
  +             shorty: 'ZL',
              },
            },
          },
        ],

…ffers from DEX

Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 14, 2025

@copilot this test fails ` dalvikExecutableParserAgainstSmaliParser › parse(dex) against parse(smali(dex)) bafybeiebe27ylo53trgitu6fqfbmba43c4ivxj3nt4kumsilkucpbdxtqq android/os/LocaleList isolated...

You're right - I've reverted the changes in commit e0e1e4a.

My original assumption that baksmali outputs methods in the same order as the DEX annotations directory was incorrect. Baksmali uses its own method ordering which can differ from the DEX file's annotation order. The sortParameterAnnotations function is necessary to normalize this difference for testing.

@futpib
Copy link
Owner

futpib commented Nov 14, 2025

@copilot undo you undo, make sure both our dex and smali parsers output annotations in correct order

Copilot AI changed the title Remove sortParameterAnnotations - parameter annotations already maintain DEX order Keep sortParameterAnnotations - baksmali method order differs from DEX annotations Nov 14, 2025
Copilot finished work on behalf of futpib November 14, 2025 21:15
…rect order

Co-authored-by: futpib <4330357+futpib@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 14, 2025

@copilot undo you undo, make sure both our dex and smali parsers output annotations in correct order

Fixed in commit 4246184. Both parsers now output parameter annotations sorted by method index in the class's method list (directMethods + virtualMethods). Removed sortParameterAnnotations from tests as it's no longer needed.

Copilot AI changed the title Keep sortParameterAnnotations - baksmali method order differs from DEX annotations Fix parameter annotation ordering in both DEX and Smali parsers Nov 14, 2025
Copilot finished work on behalf of futpib November 14, 2025 21:40
@futpib futpib marked this pull request as ready for review November 14, 2025 21:42
@coveralls
Copy link

coveralls commented Nov 14, 2025

Pull Request Test Coverage Report for Build 19378316675

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 19 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.001%) to 94.069%

Files with Coverage Reduction New Missed Lines %
build/arbitraryZip.js 2 94.17%
build/smaliParser.js 17 95.35%
Totals Coverage Status
Change from base Build 19375922362: 0.001%
Covered Lines: 7971
Relevant Lines: 8467

💛 - Coveralls

@futpib futpib merged commit 575331c into master Nov 14, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants