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

you might not like this pull request ... #2

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
cmake_minimum_required (VERSION 3.0)
project (cfg)

add_library(cfg SHARED cfg.cpp cfg_impl.cpp cbr.cpp cti.cpp app.cpp)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see why both of these are gone. On ARM I wouldn't expect cbr.cpp to work (it's a special case of branches for which all branch targets are known statically, inspired by a sample in the DR repo, cbr.c), but why are you deleting cti.cpp? cti.cpp is supposed to cover a superset of what cbr.cpp covers. Maybe we can simply disable cbr.cpp for ARM and let cti.cpp take over?

add_library(cfg SHARED cfg.cpp cfg_impl.cpp app.cpp)
find_package(DynamoRIO REQUIRED)
configure_DynamoRIO_client(cfg)
use_DynamoRIO_extension(cfg "drmgr")
Expand Down
17 changes: 17 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
all:
mkdir -p build
cd build && cmake .. -DDynamoRIO_DIR="$(DYNAMORIO_HOME)/cmake" && make

install: all
install -d /usr/local/lib/dynamorio
install build/libcfg.so /usr/local/lib/dynamorio
install drcfg.sh /usr/local/bin/

save: all
mkdir -p bin/`uname -m`
cp -v build/lib*.so bin/`uname -m`
-@svn add bin/`uname -m` 2> /dev/null
-@svn add bin/`uname -m`/*.so 2> /dev/null

clean:
rm -rf build
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Options for `drcfg` are shown below:
-no_cbr [ false] Don't count conditional branch instructions
-no_cti [ false] Don't count control transfer instructions
-output [ ""] Output results to file
-txt [ false] output text instead of json, with many doubles
```

# How to Build
Expand Down
6 changes: 6 additions & 0 deletions app.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
#ifndef APP_H_
#define APP_H_
#include "dr_api.h"
#include "drmgr.h"
#include "droption.h"

extern droption_t<bool> txt;
extern droption_t<std::string> output;

void app_init(void);
bool app_should_ignore_tag(void *tag);
Expand Down
Binary file added bin/armv7l/libcfg.so
Binary file not shown.
93 changes: 87 additions & 6 deletions cfg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,91 @@ static droption_t<bool> no_cti
(DROPTION_SCOPE_CLIENT, "no_cti", false,
"Don't count control transfer instructions", "");

static droption_t<std::string> output
droption_t<std::string> output
(DROPTION_SCOPE_CLIENT, "output", "",
"Output results to file", "");

droption_t<bool> txt
(DROPTION_SCOPE_CLIENT, "txt", false,
"Print text output while running instead of json at the end of the run - you will have to take care of the doubles youself", "");

static void *mutex;
static int tcls_idx;
static FILE *out = stdout;

typedef struct {
void *addr;
} per_thread_t;


void
dr_exit(void)
{
if (output.get_value() == "")
std::cout << std::setw(2) << construct_json() << std::endl;
else {
std::ofstream ofs(output.get_value());
ofs << std::setw(2) << construct_json() << std::endl;
if (!txt.get_value()) {
if (output.get_value() == "")
std::cout << std::hex << std::setw(2) << construct_json() << std::endl;
else {
std::ofstream ofs(output.get_value());
ofs << std::hex << std::setw(2) << construct_json() << std::endl;
}
}
drmgr_exit();
}

void
event_thread_context_init(void *drcontext, bool new_depth)
{
per_thread_t *data;

if (new_depth) {
data = (per_thread_t *) dr_thread_alloc(drcontext, sizeof(per_thread_t));
drmgr_set_cls_field(drcontext, tcls_idx, data);
} else {
data = (per_thread_t *) drmgr_get_cls_field(drcontext, tcls_idx);
}
memset(data, 0, sizeof(*data));
}

void
event_thread_context_exit(void *drcontext, bool thread_exit)
{
if (thread_exit) {
per_thread_t *data = (per_thread_t *) drmgr_get_cls_field(drcontext, tcls_idx);
dr_thread_free(drcontext, data, sizeof(per_thread_t));
drmgr_set_cls_field(drcontext, tcls_idx, NULL);
}
}

dr_emit_flags_t
event_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t *instr,
bool for_trace, bool translating, void *user_data)
{
per_thread_t *data = (per_thread_t *) drmgr_get_cls_field(drcontext, tcls_idx);
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this is trying to re-implement cbr.cpp. I would not recommend this, as it'll probably be easier to fix cbr.cpp and adapt it for ARM (and potentially re-contribute the fix towards the cbr.c sample in the DynamoRIO repo which should exhibit the same problems).

If you do not have the time for this, I can take a stab at it within the next week or so.

Copy link
Owner

Choose a reason for hiding this comment

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

Another thing--I'm wondering if this works in general. DynamoRIO fires an event_app_instruction event the first time it sees an instruction, but not subsequently. Consider the following control flow:

  a
 / \
b   c
 \ /
  d

It's a simple if-then-else statement in C. However, if we record the edge b-d, then on a subsequent execution from path a-c-d I wouldn't expect the DynamoRIO event to fire when we execute the instructions in basic block d. Does this work for you?

Copy link
Author

Choose a reason for hiding this comment

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

just reading this after reading & answering your email - just returned from vacation :)

actually, you have a point that your a -> b|c ->d might only log one path. that might be the case.
I will investigate and give you a response on that. if it is like you think, then yes, my approach is broken. if it work like I hope or thought it would work, then it would be the easiest solution.

Copy link
Author

@vanhauser-thc vanhauser-thc Apr 16, 2018

Choose a reason for hiding this comment

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

OK, done.

I did a comparison to drcfg in your repository (called "before.txt") and my cloned repository with the changes (called "after.txt"). I disabled random address space, and traced "unrar -inul p test.rar"

# ls -l after.txt before.txt
-rw-r--r-- 1 root root 324991 Apr 16 16:58 after.txt
-rw-r--r-- 1 root root 325064 Apr 16 16:58 before.txt
# wc -l after.txt before.txt
 20320 after.txt
 20325 before.txt
 40645 total

this looks like 99% similar results.

if your assumption would hold true, then for every source, there would only be one target recorded. however I find in the my patched results even results with multiple entries, e.g.:

      "address": 140737351486947,
      "targets": [
        140737324874432,
        4205056,
        140737333003728,
        140737329276560,
        140737327079680
      ]

so it looks like my approach is working. I would feel more sure if you would verify with your own tests though :)

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, will take a look w/in the next few days :)

Copy link
Owner

@toshipiazza toshipiazza Apr 17, 2018

Choose a reason for hiding this comment

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

Consider the following code: cbr.zip

It has the diamond shape, like I mentioned above. This PR yields the following output:

{
  "branches": [
    {
      "address": 40008a,
      "targets": [
        40008c
      ]
    },
    {
      "address": 400081,
      "targets": [
        400085,
        400083
      ]
    }
  ]
}

Clearly we're missing one of the targets (to the done branch, I think).

if (data->addr != NULL) {
dr_mutex_lock(mutex);
if (txt.get_value()) {
fprintf(out, "INDIRECT jmp/call from %p to %p\n", data->addr, tag);
} else {
safe_insert((uintptr_t)data->addr, (uintptr_t)tag);
}
dr_mutex_unlock(mutex);
data->addr = NULL;
}

if (!drmgr_is_last_instr(drcontext, instr))
return DR_EMIT_DEFAULT;
if (app_should_ignore_tag(tag))
return DR_EMIT_DEFAULT;

if (!no_cbr.get_value() && instr_is_cbr(instr))
data->addr = instr_get_app_pc(instr);
else if (!no_cti.get_value() && instr_is_cbr(instr))
data->addr = instr_get_app_pc(instr);

if (app_should_ignore_tag(tag))
return DR_EMIT_DEFAULT;
}

DR_EXPORT
void
dr_client_main(client_id_t id, int argc, const char *argv[])
Expand All @@ -46,9 +115,21 @@ dr_client_main(client_id_t id, int argc, const char *argv[])

app_init();
drmgr_init();

if (output.get_value() != "") {
if ((out = fopen(output.get_value().c_str(), "w")) == NULL)
DR_ASSERT_MSG(false, "can not write -outputfile");
}

mutex = dr_mutex_create();
tcls_idx = drmgr_register_cls_field(event_thread_context_init, event_thread_context_exit);
DR_ASSERT_MSG(tcls_idx != -1, "memory problems");
drmgr_register_bb_instrumentation_event(NULL, event_app_instruction, NULL);
/*
if (!no_cbr.get_value())
drmgr_register_bb_instrumentation_event(NULL, cbr_event_app_instruction, NULL);
if (!no_cti.get_value())
drmgr_register_bb_instrumentation_event(NULL, cti_event_app_instruction, NULL);
*/
dr_register_exit_event(dr_exit);
}
6 changes: 5 additions & 1 deletion cfg_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,20 @@ using nlohmann::json;

static droption_t<bool> racy
(DROPTION_SCOPE_CLIENT, "racy", false,
"Perform racy hashtable insertion", "");
"unused option", "");

static std::unordered_map<uintptr_t, std::unordered_set<uintptr_t>> cbr;
static std::mutex mtx;

void
safe_insert(uintptr_t src, uintptr_t trg)
{
/*
if (!racy.get_value()) {
std::lock_guard<std::mutex> g(mtx);
cbr[src].insert(trg);
} else
*/
cbr[src].insert(trg);
}

Expand All @@ -42,10 +44,12 @@ construct_json_impl()
json
construct_json()
{
/*
if (!racy.get_value()) {
std::lock_guard<std::mutex> g(mtx);
return construct_json_impl();
} else
*/
return construct_json_impl();
}

Expand Down
13 changes: 12 additions & 1 deletion cti.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
#include "cfg_impl.h"
#include "app.h"
#include "droption.h"

#include <iostream>
#include <sstream>
#include <fstream>
#include <iomanip>
#include <cstdint>

static droption_t<bool> instrument_ret
Expand All @@ -14,6 +17,14 @@ static void
at_cti(uintptr_t src, uintptr_t targ)
{
safe_insert(src, targ);
if (txt.get_value()) {
// if (output.get_value() == "") {
std::cout << "INDIRECT jmp/call from " << std::hex << src << " to " << targ << std::endl;
/* } else {
std::ofstream ofs(output.get_value());
ofs << "INDIRECT from " << std::hex << src << " to " << targ << std::endl;
}
*/ }
}

dr_emit_flags_t
Expand Down
16 changes: 16 additions & 0 deletions drcfg.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#/bin/sh
VER=32
uname -m 2> /dev/null | grep -q 64 > /dev/null 2>&1 && VER=64
HELP=yes
for i in $*; do
test " $i" = " --" && HELP=
done
test -n "$HELP" && {
echo "Syntax: $0 [options] -- /foo/binary -opts"
echo Options:
$DYNAMORIO_HOME/bin$VER/drrun -c /usr/local/lib/dynamorio/libcfg.so -h -- /bin/sh
exit 1
}
echo "[drcfg] using DynamoRIO"
$DYNAMORIO_HOME/bin$VER/drrun -c /usr/local/lib/dynamorio/libcfg.so $*
echo "[drcfg] done"