i#1834 memval Part 4: Adds memval sample #2174

toshipiazza · 2017-02-13T05:52:39Z

Introduces a sample client which inserts instrumentation after the
current instruction to dereference app memory, and to fill a per-thread
buffer.

Fixes #1834

Introduces a sample client which inserts instrumentation after the current instruction to dereference app memory, and to fill a per-thread buffer. Fixes DynamoRIO#1834

toshipiazza · 2017-02-13T05:54:36Z

Note I'm adding ARM support currently, so I'm still working out some kinks. x86 works fine however.

Also I will look at the Windows build failure in the morning.

toshipiazza · 2017-02-14T04:45:59Z

I'm currently looking at the ARM failure in memval_simple, but it seems to be a problem with how I'm using dr_insert_clean_call. If I apply the following patch,

diff --git a/ext/drx/drx_buf.c b/ext/drx/drx_buf.c
index 0caf226..ca28fd3 100644
--- a/ext/drx/drx_buf.c
+++ b/ext/drx/drx_buf.c
@@ -696,6 +696,7 @@ safe_memcpy(drx_buf_t *buf, void *src, size_t len)
     size_t written;
     bool ok;

+    dr_printf("safe_memcpy(buf: %p, src: %p, len: %d)\n", buf, src, len);
     DR_ASSERT_MSG(buf->buf_size >= len,
                   "buffer was too small to fit requested memcpy() operation");
     /* try to perform a safe memcpy */
@@ -711,6 +712,12 @@ safe_memcpy(drx_buf_t *buf, void *src, size_t len)
     BUF_PTR(data->seg_base, buf->tls_offs) += len;
 }

+static void
+dump_arg(void *foo)
+{
+    dr_printf("%p\n", foo);
+}
+
 DR_EXPORT
 void
 drx_buf_insert_buf_memcpy(void *drcontext, drx_buf_t *buf, instrlist_t *ilist,
@@ -722,6 +729,9 @@ drx_buf_insert_buf_memcpy(void *drcontext, drx_buf_t *buf, instrlist_t *ilist,
         opnd_t buf_opnd = OPND_CREATE_INTPTR(buf);
         opnd_t src_opnd = opnd_create_reg(src);
         opnd_t len_opnd = OPND_CREATE_INTPTR((short)len);
+        dr_insert_clean_call(drcontext, ilist, where, (void *)dump_arg, false, 1, buf_opnd);
+        dr_insert_clean_call(drcontext, ilist, where, (void *)dump_arg, false, 1, src_opnd);
+        dr_insert_clean_call(drcontext, ilist, where, (void *)dump_arg, false, 1, len_opnd);
         dr_insert_clean_call(drcontext, ilist, where, (void *)safe_memcpy, false, 3,
                              buf_opnd, src_opnd, len_opnd);
     } else {

I see the following behaviour:

./bin32/drrun -c ./api/bin/libmemval_simple.so -- ls
...
0x4c4d9120 # buf
0x7ecbff54 # src
0x00000014 # len
safe_memcpy(buf: 0x4c4d9120, src: 0x00000014, len: 20)
<(1+x) Handling our fault in a TRY at 0x54da7694>
<Application /bin/ls (14389).  DynamoRIO Sample Client 'memval_simple' internal crash at PC 0x54da7694.  Please report this at http://dynamorio.org/issues.  Program aborted.
Received SIGSEGV at pc 0x54da7694 in thread 14389
...

We're clearly crashing because src is 0x14 (the same as the size), but if we look at the output from the clean calls earlier we see that src is clearly a valid pointer! Even more mysteriously, I don't see this problem in the drx_buf test (it passes for me). What's going on?

derekbruening · 2017-02-14T05:02:18Z

Is there a bug in ARM's clean call impl for 3 args? Maybe all the tests of it only pass 1 or 2 args?

toshipiazza · 2017-02-14T05:10:42Z

Hmm, when looking through the AArchXX implementation of insert_parameter_preparation(), I see the following comment:

/* FIXME i#1551, i#1569: we only implement naive parameter preparation,
 * where args are all regs or immeds and do not conflict with param regs.
 */

If I remember correctly, memval_simple crashed on the second usage of drx_buf_insert_buf_memcpy(), which used r2 as the src register. This also corresponds to the third argument register (i.e. len?). Will double check that this was the case.

More Info

Confirmed, $r2 was used as the src register when the app crashed, whereas $r1 was used as the src register previously when the app didn't crash. $r1 already corresponds to argument register 2 by pure coincidence, so that's probably why it didn't crash the first time.

Also, I'm pretty sure we didn't crash on drx_buf-test.dll.c, because we hardcoded the scratch registers to be $r3 and $r4. $r3 is argument register 4, but we never use a clean call that has that many arguments, so no crash.

derekbruening

Looks good overall, just some minor style-type comments

derekbruening · 2017-02-13T21:34:32Z

api/samples/memval_simple.c

+ * Records and dumps app write addresses, and their corresponding written values.
+ *
+ * (1) It fills two per-thread-buffers with inlined instrumentation.
+ * (2) Once the buffer has been filled up, a fault handler will redirect execution


s/buffer has/buffers have/

derekbruening · 2017-02-13T21:35:02Z

api/samples/memval_simple.c

+ * - inserting instrumentation after the current instruction to read the value
+ *   written by it.
+ * - the use of drutil_expand_rep_string() to expand string loops to obtain
+ *   every memory reference,


nit: some list items end in . while some end in ,

derekbruening · 2017-02-14T22:24:40Z

api/samples/memval_simple.c

+    char *hexstring = hex_buf, *needle = hex_buf;
+
+    for (i = mem_ref->size - 1; i >= 0; --i)
+        needle += dr_snprintf(needle, 2*mem_ref->size+1-2*i, "%02x", write_base[i]);


Safer to use the returned value for the size: so 2*mem_ref->size+1-(needle-hex_buf)

derekbruening · 2017-02-14T22:25:54Z

api/samples/memval_simple.c

+    /* write the memrefs to disk */
+    for (mem_ref = trace_base; mem_ref < trace_ptr; mem_ref++) {
+        /* Each memref in the trace buffer has an "associated" write in the write buffer.
+         * We pull mem_reg->size bytes from the write buffer, and assert we haven't yet


s/mem_reg/mem_ref/

derekbruening · 2017-02-14T22:45:32Z

api/samples/memval_simple.c

+    bool ok;
+
+    if (drreg_reserve_register(drcontext, ilist, where, NULL, &reg_tmp)
+            != DRREG_SUCCESS) {


style: indent should be under 'd' of 'drreg' -- but prob fits on line above? I wish github let me set a max line width and show a marker there.

Done, I checked and it doesn't fit (by one char).

derekbruening · 2017-02-14T22:45:45Z

api/samples/memval_simple.c

+        return DR_REG_NULL;
+    }
+    if (drreg_reserve_register(drcontext, ilist, where, NULL, &reg_ptr)
+            != DRREG_SUCCESS) {


derekbruening · 2017-02-14T22:46:14Z

api/samples/memval_simple.c

+         * will get clobbered on ARM.
+         */
+        if (drreg_reserve_register(drcontext, ilist, where, NULL, &reg_addr)
+                != DRREG_SUCCESS) {


derekbruening · 2017-02-14T22:47:22Z

api/samples/memval_simple.c

+    ushort stride = (ushort)drutil_opnd_mem_size_in_bytes(memref, write);
+
+    if (drreg_reserve_register(drcontext, ilist, where, NULL, &reg_ptr)
+            != DRREG_SUCCESS) {


toshipiazza · 2017-02-15T04:22:59Z

Should I wait for the FIXME in dr_insert_clean_call to be resolved (and should I be the one to solve it?) before I merge in my changes? If I merge these in, then we'll see drx_buf-test failing on ARM.

derekbruening · 2017-02-15T04:50:26Z

Should I wait for the FIXME in dr_insert_clean_call to be resolved (and should I be the one to solve it?) before I merge in my changes? If I merge these in, then we'll see drx_buf-test failing on ARM.

It would be great if you fixed it :) (Via separate pull request.) Or you could disable for ARM the part of drx_buf-test that will fail and in drx.h mark the routine that uses the clean call as NYI on ARM.

derekbruening · 2017-02-15T23:06:38Z

For future pull requests, please take a look at the new workflow: we're using feature branches in the DR repo itself now, so no private clone.

toshipiazza · 2017-02-16T05:02:14Z

With regards to maybe fixing up dr_insert_clean_call, I'll do more analysis and see what needs to be done (probably before the end of the week) before determining whether I want to just disable ARM here or address the FIXME in a separate PR.

toshipiazza · 2017-02-22T05:03:38Z

FYI Because I'm a little busy with school atm, I'll just disable the build of memval_simple on ARM for now, and later this week I'll look into cleaning up dr_insert_clean_call to some degree.

toshipiazza · 2017-02-22T08:11:29Z

Hmm it's been a couple hours and travis says all tests passed here but it looks like the github UI says it hasn't yet finished. Is there a problem or is there just some lag?

derekbruening · 2017-02-22T14:00:28Z

I have never seen this before. I've seen the Mac build sit in a queue waiting for a free Travis machine for hours, but here Mac has run. I'll go ahead and override. Maybe we should file a bug against Travis.

toshipiazza added 2 commits February 12, 2017 22:00

i#1834 memval Part 4: Adds memval sample

12ce9c5

Introduces a sample client which inserts instrumentation after the current instruction to dereference app memory, and to fill a per-thread buffer. Fixes DynamoRIO#1834

Adds ARM support for memval_simple.c

54a5478

toshipiazza requested a review from derekbruening February 13, 2017 05:55

Fixes MS2010 build

4b4e62a

derekbruening requested changes Feb 14, 2017

View reviewed changes

Fixes formatting and comment issues

8fe13a8

derekbruening approved these changes Feb 15, 2017

View reviewed changes

Disables memval_simple on ARM

6c4d59e

Merge branch 'master' into i#1834

8af115b

derekbruening approved these changes Feb 22, 2017

View reviewed changes

derekbruening mentioned this pull request Feb 22, 2017

AArchXX clean calls handle far too few use cases, blocking tool development #2210

Open

derekbruening merged commit 0842214 into DynamoRIO:master Feb 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i#1834 memval Part 4: Adds memval sample #2174

i#1834 memval Part 4: Adds memval sample #2174

toshipiazza commented Feb 13, 2017

toshipiazza commented Feb 13, 2017 •

edited

toshipiazza commented Feb 14, 2017

derekbruening commented Feb 14, 2017

toshipiazza commented Feb 14, 2017 •

edited

derekbruening left a comment

derekbruening Feb 13, 2017

toshipiazza Feb 15, 2017

derekbruening Feb 13, 2017

toshipiazza Feb 15, 2017

derekbruening Feb 14, 2017

toshipiazza Feb 15, 2017

derekbruening Feb 14, 2017

toshipiazza Feb 15, 2017

derekbruening Feb 14, 2017

toshipiazza Feb 15, 2017

derekbruening Feb 14, 2017

toshipiazza Feb 15, 2017

derekbruening Feb 14, 2017

toshipiazza Feb 15, 2017

derekbruening Feb 14, 2017

toshipiazza Feb 15, 2017

toshipiazza commented Feb 15, 2017

derekbruening commented Feb 15, 2017

derekbruening commented Feb 15, 2017

toshipiazza commented Feb 16, 2017

toshipiazza commented Feb 22, 2017

toshipiazza commented Feb 22, 2017

derekbruening commented Feb 22, 2017

i#1834 memval Part 4: Adds memval sample #2174

i#1834 memval Part 4: Adds memval sample #2174

Conversation

toshipiazza commented Feb 13, 2017

toshipiazza commented Feb 13, 2017 • edited

toshipiazza commented Feb 14, 2017

derekbruening commented Feb 14, 2017

toshipiazza commented Feb 14, 2017 • edited

More Info

derekbruening left a 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

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

toshipiazza commented Feb 15, 2017

derekbruening commented Feb 15, 2017

derekbruening commented Feb 15, 2017

toshipiazza commented Feb 16, 2017

toshipiazza commented Feb 22, 2017

toshipiazza commented Feb 22, 2017

derekbruening commented Feb 22, 2017

toshipiazza commented Feb 13, 2017 •

edited

toshipiazza commented Feb 14, 2017 •

edited