Workaround issue with DebugSymbol #48

alvarofe · 2017-02-19T12:10:56Z

Dunno if this is the best solution but I noticed that when using dtf to trace calls instead of

write 1,"lala\no\n",5

I was getting

0x7fff98084910 1, "lala\no\n", 5

This can be a hassle when you are tracing many functions. DebugSymbol should be doing this, but as far I know it does not work so is just a dirty workaround.

If this is not good javascript just tell me quite noob tbh.

alvarofe · 2017-02-19T12:14:34Z

Here the issue with DebugSymbol frida/frida-gum#186

oleavr · 2017-02-19T13:25:55Z

Interesting. What does DebugSymbol.fromAddress(Module.findExportByName(null, 'write')) return if you run it in the Frida REPL on your system? (To install the REPL: pip install frida and run frida -p 0, which gives you a system-session meaning the JS gets executed inside of Frida itself, so you don't have to attach to any process.)

alvarofe · 2017-02-19T15:23:51Z

mm. It does work from REPL but it wasn't on r2frida side

[Local::ProcName::Tweetbot]-> DebugSymbol.fromAddress(Module.findExportByName(null, 'write'))
{
    "address": "0x7fff98084910",
    "fileName": "",
    "lineNumber": 0,
    "moduleName": "libsystem_kernel.dylib",
    "name": "write"
}

alvarofe · 2017-02-19T15:26:19Z

air:r2 alvaro$ r2 frida://cat
Warning: r2frida cannot initialize mjolner
 -- Change the block size with 'b <block-size>'. In visual mode you can also enter radare2 command pressing the ':' key (like vi does)
[0x00000000]> \is write
0x7fff98084910
[0x00000000]> s 0x7fff98084910
[0x7fff98084910]> \dtf ^izi
true
[0x7fff98084910]> 0x7fff98084910 1,"lala\n",5

alvarofe · 2017-02-19T15:27:53Z

This from REPL fails

[Local::ProcName::cat]-> DebugSymbol.fromAddress(ptr(0x7fff98084910));
{
    "address": "0x7fff98084910",
    "fileName": null,
    "lineNumber": null,
    "moduleName": null,
    "name": null
}

alvarofe · 2017-02-19T15:33:09Z

Is kind of random behavior. With Tweetbot works but with cat it doesn't.

http://asciinema.org/a/dfk9e430250x7gbm217zmcgch

trufae · 2017-02-19T19:06:32Z

src/agent/index.js

@@ -961,7 +961,19 @@ function traceFormat(args) {
  const traceOnEnter = format.indexOf('^') !== -1;
  const traceBacktrace = format.indexOf('+') !== -1;

-  const at = DebugSymbol.fromAddress(ptr(address)) || '' + ptr(address);
+  var module = Process.enumerateModulesSync()[0].name;


use const instead of var here

trufae · 2017-02-19T19:06:37Z

src/agent/index.js

+  var module = Process.enumerateModulesSync()[0].name;
+  var imports = Module.enumerateImportsSync(module);
+  var at = '';
+  for (var index = 0; index < imports.length; index++) {


use let instead of var

trufae · 2017-02-19T19:06:41Z

src/agent/index.js

@@ -961,7 +961,19 @@ function traceFormat(args) {
  const traceOnEnter = format.indexOf('^') !== -1;
  const traceBacktrace = format.indexOf('+') !== -1;

-  const at = DebugSymbol.fromAddress(ptr(address)) || '' + ptr(address);
+  var module = Process.enumerateModulesSync()[0].name;
+  var imports = Module.enumerateImportsSync(module);


imports are const

trufae · 2017-02-19T19:07:34Z

src/agent/index.js

+  var module = Process.enumerateModulesSync()[0].name;
+  var imports = Module.enumerateImportsSync(module);
+  var at = '';
+  for (var index = 0; index < imports.length; index++) {


you can iterate here with for (let import of imports) { if (imp.address === address) {

trufae · 2017-02-19T19:07:39Z

src/agent/index.js

+  var imports = Module.enumerateImportsSync(module);
+  var at = '';
+  for (var index = 0; index < imports.length; index++) {
+    if (imports[index].address == address) {


Because of using == instead of === it does work. Maybe @oleavr that is the issue with DebugSymbol?

trufae · 2017-02-19T19:08:06Z

src/agent/index.js

+      break;
+    }
+  }
+  if (at == '') {


use ===, but better check for undefined. what if the import name is '' ? :P

trufae · 2017-02-19T19:08:23Z

src/agent/index.js

@@ -961,7 +961,19 @@ function traceFormat(args) {
  const traceOnEnter = format.indexOf('^') !== -1;
  const traceBacktrace = format.indexOf('+') !== -1;

-  const at = DebugSymbol.fromAddress(ptr(address)) || '' + ptr(address);
+  var module = Process.enumerateModulesSync()[0].name;


move this logic into a separate function

trufae · 2017-02-19T19:09:05Z

src/agent/index.js

+  if (at == '') {
+    '' + ptr(address);
+  }
+  //const at = DebugSymbol.fromAddress(ptr(address)) || '' + ptr(address);


remove leftover

trufae · 2017-02-19T19:11:54Z

src/agent/index.js

+    }
+  }
+  if (at == '') {
+    '' + ptr(address);


this line does nothing

alvarofe · 2017-02-19T19:41:24Z

src/agent/index.js

+  const module = Process.enumerateModulesSync()[0].name;
+  const imports = Module.enumerateImportsSync(module);
+  for (let imp of imports) {
+    if (imp.address == address) {


So == is needed to make a type conversion? If I use === it doesn't work

you can also use compare method for native pointers/addresses

[Local::ProcName::cat]-> imports[0].address "0x7fffb458f374" [Local::ProcName::cat]-> imports[0].address.compare function [Local::ProcName::cat]-> imports[0].address.compare("0x0") // better to use `isNull()` 1 [Local::ProcName::cat]-> imports[0].address.compare(imports[0].address) 0 [Local::ProcName::cat]-> imports[0].address.compare(imports[1].address) -1

Compare doesn't work.

[Local::ProcName::cat]-> imports[36].address.compare("0x7fff98084910") 0 [Local::ProcName::cat]-> imports[36].address == "0x7fff98084910" true [Local::ProcName::cat]-> imports[36] { "address": "0x7fff98084910", "module": "/usr/lib/libSystem.B.dylib", "name": "write", "type": "function" }

Javascript ¯\(ツ)/¯

oleavr · 2017-02-19T20:34:30Z

@alvarofe Yeah always use strict equality checking, the others shouldn't be used IMO. In this case though, the value is a NativePointer, so .equals() should be used. By the way, shouldn't this logic be a fallback when DebugSymbol fails?

oleavr · 2017-02-19T20:36:16Z

Btw, if you feel like a little tour of non-strict equality craziness, check out this lightning talk:
https://www.destroyallsoftware.com/talks/wat

alvarofe · 2017-02-19T21:39:28Z

I should review other places when DebugSymbol is used in case it fails

radare · 2017-02-20T04:36:53Z

compare(ptr("0x...

…

On 19 Feb 2017, at 20:41, Álvaro Felipe Melchor ***@***.***> wrote: @alvarofe commented on this pull request. In src/agent/index.js: > @@ -947,6 +947,22 @@ function getPtr(p) { return Module.findExportByName(null, p); } +function nameFromAddress(address) { + var at = ''; + const module = Process.enumerateModulesSync()[0].name; + const imports = Module.enumerateImportsSync(module); + for (let imp of imports) { + if (imp.address == address) { So == is need to make a type conversion? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

oleavr · 2017-02-20T10:50:16Z

src/agent/index.js

@@ -91,6 +91,24 @@ const RTLD_GLOBAL = 0x8;
 const RTLD_LAZY = 0x1;
 const allocPool = {};

+function nameFromAddress(address) {


Missing space between function name and opening parenthesis as per semistandard.

oleavr · 2017-02-20T10:54:35Z

src/agent/index.js

@@ -91,6 +91,24 @@ const RTLD_GLOBAL = 0x8;
 const RTLD_LAZY = 0x1;
 const allocPool = {};

+function nameFromAddress(address) {
+  let at =  DebugSymbol.fromAddress(ptr(address)).name


Extra space and missing semicolon.

By guaranteeing that this function is always called with a NativePointer (see suggestion below), we can drop ptr() here so it's just:

let at = DebugSymbol.fromAddress(address).name;

oleavr · 2017-02-20T10:56:58Z

src/agent/index.js

@@ -960,8 +978,8 @@ function traceFormat(args) {
  }
  const traceOnEnter = format.indexOf('^') !== -1;
  const traceBacktrace = format.indexOf('+') !== -1;
+  const at = nameFromAddress (address);


It would be simpler to always pass a NativePointer value to this function, by changing the else above from:

var address = offset;

to:

var address = ptr(offset);

(Note to self: we should refactor so offset is always a NativePointer.)

oleavr · 2017-02-20T10:58:30Z

src/agent/index.js

+    const module = Process.enumerateModulesSync()[0].name;
+    const imports = Module.enumerateImportsSync(module);
+    for (let imp of imports) {
+      if (imp.address.equals(ptr(address))) {


Same with this ptr().

oleavr · 2017-02-20T10:59:59Z

src/agent/index.js

+      }
+    }
+    if (at === null) {
+      at = '' + ptr(address);


And this ptr().

alvarofe · 2017-02-20T22:25:15Z

I had only time to update the PR. I couldn't look at other instances of DebugSymbol.

Regarding code style are you using https://github.com/Flet/semistandard ? just to make sure the next time I upload code in here.

Regarding compare @dweinstein you were right, I was using a string instead of NativePointer so my mistake.

oleavr · 2017-02-21T21:48:11Z

src/agent/index.js

@@ -91,6 +91,24 @@ const RTLD_GLOBAL = 0x8;
 const RTLD_LAZY = 0x1;
 const allocPool = {};

+function nameFromAddress (address) {
+  let at = DebugSymbol.fromAddress(address).name


Missing semicolon.

oleavr · 2017-02-21T21:48:32Z

src/agent/index.js

+      }
+    }
+    if (at === null) {
+      at = '' + address;


at = address.toString(); would be clearer.

this is also done in several other places in this js, so i would go to change this in a separate pr

trufae · 2017-02-21T21:50:59Z

src/agent/index.js

@@ -91,6 +91,24 @@ const RTLD_GLOBAL = 0x8;
 const RTLD_LAZY = 0x1;
 const allocPool = {};

+function nameFromAddress (address) {


if fromAddress() return null the .name will throw an exception that is not catched. i think that this code should be written in a different way. But Frida should already resolve those symbols . so it's a bug in Frida. Can you provide a reproducer? cc @oleavr

trufae · 2017-02-21T21:55:01Z

The problem here is that Frida is not exposing the address of the imports in the PLT/STUB inside the binary, so the symbols that hasnt been executed yet cannot be resolved. that's why you cant resolve write in cat because it's on hold in read.

One solution that comes to my mind will be to make Frida resolve the PLT symbols with the DebugSymbol API, or maybe provide an api to do that without having to iterate in js

trufae · 2017-02-21T22:04:44Z

nvm happens with read too.

radare · 2017-02-22T08:14:55Z

Thats because frida uses CoreSymbolication framework which depends on CoreFoundation and those libs are not loaded in cat. Its an issue in Frida that should be addressed there. We can have a workaround but I would prefer to have this solved using ApiResolver or making Frida use ApiResolver in case the symbolicator cant be instantiated.

…

On 21 Feb 2017, at 23:04, Sergi Àlvarez i Capilla ***@***.***> wrote: nvm happens with read too. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

alvarofe · 2017-02-27T09:54:52Z

I agree on that, do not have any clue about Frida's internals but If I can help to sort this out let me know. Ok to close this PR and move the issue into Frida? Where is this in handled in Frida within frida-gum?

trufae · 2017-08-28T19:59:20Z

i'll merge this workaround, and do some tweaks later. but the fix should be done in Frida, not in r2frida, as long as we have other workarounds for Frida in r2frida, it's ok for me to have this merged to make r2frida useful outside mac

alvarofe · 2017-08-28T21:33:14Z

I agree on that but Frida code is out of my reach for the time being, I asked where to open the issue, basically in which frida's repo. But if you want to include this temporary is ok for me

trufae · 2017-08-30T17:53:25Z

src/agent/index.js

+  if (at === null) {
+    const module = Process.enumerateModulesSync()[0].name;
+    const imports = Module.enumerateImportsSync(module);
+    for (let imp of imports) {


you can use Process.findModuleByAddress(address) to avoid having to iterate over all modules to find which import falls in this address

also, iterate over the Exports too

trufae · 2017-08-30T18:17:18Z

Fixed the comments and merged from 8e5401d

trufae reviewed Feb 19, 2017

View reviewed changes

src/agent/index.js Outdated

break;

}

}

if (at == '') {

Copy link

Member

trufae Feb 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ===, but better check for undefined. what if the import name is '' ? :P

trufae reviewed Feb 19, 2017

View reviewed changes

src/agent/index.js Outdated

}

}

if (at == '') {

'' + ptr(address);

Copy link

Member

trufae Feb 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line does nothing

alvarofe commented Feb 19, 2017

View reviewed changes

oleavr requested changes Feb 20, 2017

View reviewed changes

alvarofe added 5 commits February 20, 2017 23:17

Workaround issue with DebugSymbol

fdec4f6

First review round

f1555e6

Second round of review

fa37d5e

Fix at when name is not found

2c40b33

Fix oleavr's comments: pass ptr(address) to nameFromAddress

86bb8f0

alvarofe force-pushed the fix branch from 6f4a3f2 to 86bb8f0 Compare February 20, 2017 22:20

oleavr reviewed Feb 21, 2017

View reviewed changes

trufae reviewed Feb 21, 2017

View reviewed changes

radare mentioned this pull request Mar 13, 2017

DebugSymbol without CoreSymbolication frida/frida-gum#202

Open

Merge branch 'master' into fix

59c34ad

trufae reviewed Aug 30, 2017

View reviewed changes

trufae closed this Aug 30, 2017

Workaround issue with DebugSymbol #48

Workaround issue with DebugSymbol #48

Conversation

alvarofe commented Feb 19, 2017

alvarofe commented Feb 19, 2017

oleavr commented Feb 19, 2017 • edited

alvarofe commented Feb 19, 2017 • edited

alvarofe commented Feb 19, 2017

alvarofe commented Feb 19, 2017 • edited

alvarofe commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarofe Feb 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweinstein Feb 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleavr commented Feb 19, 2017

oleavr commented Feb 19, 2017

alvarofe commented Feb 19, 2017

radare commented Feb 20, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleavr Feb 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarofe commented Feb 20, 2017

Choose a reason for hiding this comment

oleavr Feb 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trufae commented Feb 21, 2017

trufae commented Feb 21, 2017

radare commented Feb 22, 2017 via email

alvarofe commented Feb 27, 2017

trufae commented Aug 28, 2017

alvarofe commented Aug 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trufae commented Aug 30, 2017

oleavr commented Feb 19, 2017 •

edited

alvarofe commented Feb 19, 2017 •

edited

alvarofe commented Feb 19, 2017 •

edited

alvarofe Feb 19, 2017 •

edited

dweinstein Feb 19, 2017 •

edited

oleavr Feb 20, 2017 •

edited

oleavr Feb 21, 2017 •

edited

alvarofe commented Aug 28, 2017 •

edited