Web UI sometimes fails to display trace for MT issues

[0xedward]

Steps to Reproduce:

1. Download the zip of a SAPP db containing the MT run to help you reproduce the issue and the source code of what MT was run on

2. Extract both zip files and run the following command to get started looking into this in SAPP

python3 -m sapp.cli --database-name sapp.db --source-directory repo/ --debug

3. Visit http://localhost:3000 and click on Issue 2 in Run 1
   
4. The following trace should print
   

Here are the relevant local vars when the exception is thrown:

• lines:

[
    "package com.example.myapplication;",
    "",
    "import android.util.Log;",
    "import java.io.IOException;",
    "/* loaded from: classes2.dex */",
    "public class CodeExecuteUtil {",
    "    public static void execute(String str) {",
    "        try {",
    "            new ProcessBuilder(str).start();",
    "        } catch (IOException e) {",
    "            Log.e(\"CodeExecuteUtil\", \"Failed to execute\" + str);",
    "        }",
    "    }",
    "}",
    ""
]

• range:

{
    "from": {
        "line": 15,
        "ch": 0
    },
    "to": {
        "line": 15,
        "ch": 1
    }
}

As a starting point, this issue likely occurs because of a off-by-one error when referencing lines in https://github.com/facebook/sapp/blob/main/sapp/ui/frontend/src/Source.js#L40

Submitting a PR

We use the following linters internally, so to save everyone's time, please make sure you run the following linters locally and fix errors related to the files you modified before submitting a PR:

black && usort format . && flake8

To install the linters, you can run the following command:

pip install flake8 usort black==21.4b2

[abishekvashok]

@0xedward I debugged this for some time and it appears as though the issue is with the db provided (spurious information, either the db or the files have been changed after running MT) or an issue with MT itself.

What I did was tried to log issues which passes the line number to various functions till it reaches the source where we try to access the line number th line. For instance, one of the issues I logged is:

Object
__typename: "IssueQueryResultType"
callable: "Lcom/example/myapplication/MainActivity;.onCreate:(Landroid/os/Bundle;)V"
code: 3
features: ["always-via-caller-exported", "always-via-intent-data", "always-via-obscure", "always-via-obscure-taint-in-taint-out", "always-via-obscure-taint-in-taint-this", "via-caller-exported", "via-intent-data", "via-obscure", "via-obscure-taint-in-taint-out", "via-obscure-taint-in-taint-this"] (10)
filename: "com/example/myapplication/MainActivity.java"
issue_id: "2"
issue_instance_id: "2"
location: "35|1|1"
message: "User input flows into implicitly launched intent: Values from user-controlled source may eventually flow into an implicit intent and inten…"
min_trace_length_to_sinks: 1
min_trace_length_to_sources: 1
sink_names: ["Landroid/app/Activity;.startActivity:(Landroid/content/Intent;)V"] (1)
sinks: ["Partial:LaunchingComponent:b"] (1)
source_names: ["Landroid/app/Activity;.getIntent:()Landroid/content/Intent;"] (1)
sources: ["ActivityUserInput"] (1)

As you can see the issue location is at line number 35, from 0th place char to the 1st (in sense of an array of chars)
But on opening com/example/myapplication/MainActivity.java, I could only see the following 29 lines:

package com.example.myapplication;

import android.content.Intent;
import android.os.Bundle;
import android.view.View;
import android.widget.Button;
import androidx.appcompat.app.AppCompatActivity;
/* loaded from: classes2.dex */
public class MainActivity extends AppCompatActivity {
    /* access modifiers changed from: protected */
    @Override // androidx.appcompat.app.AppCompatActivity, androidx.fragment.app.FragmentActivity, androidx.activity.ComponentActivity, androidx.core.app.ComponentActivity, android.app.Activity
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        final Intent intent = getIntent();
        setContentView(2131427356);
        ((Button) findViewById(2131230792)).setOnClickListener(new View.OnClickListener() { // from class: com.example.myapplication.MainActivity.1
            @Override // android.view.View.OnClickListener
            public void onClick(View v) {
                CodeExecuteUtil.execute(intent.getStringExtra("command"));
            }
        });
        if (intent.getBooleanExtra("redirect", false)) {
            Intent redirectIntent = new Intent();
            redirectIntent.setData(intent.getData());
            startActivity(redirectIntent);
        }
    }
}

This lead me to the above conclusion. I couldn't ascertain which of the above this be the result of, but if you want, I could prevent this error from happening by pointing to the last line if in case such a catastrophe happens.

[0xedward]

I debugged this for some time and it appears as though the issue is with the db provided (spurious information, either the db or the files have been changed after running MT) or an issue with MT itself.

@abishekvashok - oh, interesting! Does this issue you mention reproduce if you do the following?

1. Run MT on the source code
2. Delete the current sapp.db you have
3. Ingest the results from the latest MT run into SAPP

[abishekvashok]

@0xedward so I did some digging up and the findings affirm my conclusion. This is if the source code provided, I assume, is same as the sample app in the repository. Otherwise I would also need the apk to run MT.

So if it is same as the sample app, all is well. Every issue has traces displayed without errors. eg) the 2nd one:

[abishekvashok]

@0xedward compiled the apk as suggested. No issues on running the newly created project with the source provided:

But however, the error is reproducible when sapp has invested the results and when file has changed. Do you want to anchor this issue to implement the "snapshot" behaviour you guys have internally? If so, I was curious to know which method you would prefer: create a pipeline step to add the files which are associated with traces to the db on a snapshots table, make freezable copies of the file and save them as archives associated with run ids, or something entirely else :)

[0xedward]

But however, the error is reproducible when sapp has invested the results and when file has changed.

@abishekvashok thanks for spending the time to find the root cause of this! It looks like our guess was right :)

Do you want to anchor this issue to implement the "snapshot" behaviour you guys have internally?

My initial thoughts are that it makes sense for us to mirror what we have internally, but I have a few concerns so I'll message a few people to see if I can sort out (e.g. SAPP is a dependency for some internal projects, UI for making it clear to our users that they are viewing a snapshot of their code, what do we do if the user doesn't specify --source-directory when they run sapp analyze, etc)

If so, I was curious to know which method you would prefer: create a pipeline step to add the files which are associated with traces to the db on a snapshots table, make freezable copies of the file and save them as archives associated with run ids, or something entirely else :)

Depending on how the internal discussions go, my guess here is creating pipeline step to snapshot files when the user runs sapp analyze makes the most sense. Though if we want to store snapshots in a table, I'm not certain what is the memory efficient and performant way of doing this. I'll see if I can find out what we do internally for this.

[abishekvashok]

@0xedward digged in deeper with suggestions following the meeting (thanks for them). We indeed have commit hash relating with each run which leads me to believe we could use it in the file connection response to the UI and if there isn't a commit hash associated with the run, either:

• show a warning if there's a conflict (too few no: of lines etc.)
• create a pipeline step to create snapshot(s) and display those instead unless there's a --source-directory option

My only concern for the second option is the creation of duplicate snapshots when the run is imported again and again and the inability of us to add another third party library to sapp because you guys have to support it internally.

[abishekvashok]

Closed via facebook@01df95d

[BitTheByte]

Hi @0xedward and @abishekvashok

I don't think this is fully fixed yet I'm still experiencing the same issue using 0.5.1

[abishekvashok]

@BitTheByte that's very unfortunate. Do you mind giving more details? Like a screenshot of the things in the console for example... we did try to check a possible few cases before closing this and would help if you could give more of a context as to when this happens. Also thanks for trying this out - really appreciate it :)

[0xedward]

Hey @BitTheByte, thanks for looking into this after the latest release! :)

Like @abishekvashok mentioned, would you be able to provide rough steps to reproduce for the issue you are still running into? Only if it's possible and you're comfortable with sharing, it would be really helpful if you could share your sapp.db and a zip of the directory you are using for the --source-directory option in the reproduction steps.

[BitTheByte]

Hi @abishekvashok and @0xedward

 $ sapp --database-name=sapp.db server --source-directory=src/main/java

Python 3.8.10
Ubuntu 20.04 WSL2
SAPP 0.5.1

Find the apk, decompiled sources, and sapp.db at https://file.io/XZaskNQFpAS1

Below is the message shown at the browser console. and a small snippet of the code causing the error

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'slice')
    at Source.js:53
    at Source.js:93
    at Array.map (<anonymous>)
    at ce (Source.js:76)
    at ue (Source.js:204)
    at $i (react-dom.production.min.js:153)
    at Ia (react-dom.production.min.js:175)
    at mc (react-dom.production.min.js:263)
    at ls (react-dom.production.min.js:246)
    at cs (react-dom.production.min.js:246)

function adjustRange(range: Range, lines: $ReadOnlyArray<string>): Range {
  // TODO(T78595608): workaround for inaccurate Pysa locations with leading and
  // trailing whitespaces.

  // Assuming all ranges are single line.
  const source = lines[range.from.line].slice(range.from.ch, range.to.ch); // << ERROR
  const leadingWhitespace = source.search(/\S/);
  const trailingWhitespace = source.length - source.trimEnd().length;
  return {
    from: {
      line: range.from.line,
      ch: range.from.ch + leadingWhitespace,
    },
    to: {
      line: range.to.line,
      ch: range.to.ch - trailingWhitespace,
    },
  };
}

Note that I'm using jadx to acquire the source code. I don't really know if this is an applicable case for this tool. however, if the source code and bytecode is not properly correlated should not crash the whole UI

Thanks for taking the time fixing and looking into this issue :)

[abishekvashok]

@BitTheByte thank you for this

@0xedward I took a look and it seems MT is reporting line numbers in an off by one fashion.While I think pysa has line numbers starting from 1, MT (maybe for this db) has for some issues 0, as line numbers.
This causes line to be -1 and hence result in a slicing error:
https://github.com/facebook/sapp/blob/b4b9be7251ba6c780c8b76fa08a1088eaf7c1ade/sapp/ui/frontend/src/Source.js#L81

const source = lines[range.from.line].slice(range.from.ch, range.to.ch); // << ERROR

I think we can fix this by modifying the MT parser pipeline step in
https://github.com/facebook/sapp/blob/b4b9be7251ba6c780c8b76fa08a1088eaf7c1ade/sapp/pipeline/mariana_trench_parser.py#L127

to

line = position.get("line", UNKNOWN_LINE)
if line != UNKNOWN_LINE:
    line = line + 1

or modifying UNKNOWN_LINE to -2 and line = position.get("line", UNKNOWN_LINE) + 1