i#1501 Port DynamoRIO to ARM: adjust label instr handling within IT blocks

Commit log for first patchset:
---------------
i#1501 Port DynamoRIO to ARM: adjust label instr handling within IT blocks

- add encode_reset_it_block to reset encode state when necessary
- reset encode state in build_bb_ilist when adding exit instr for svc.cc
- skip label instrunction in encode_in_it_block getting prev instr
- stop IT block construction on label instruction
---------------

[bruening, 2015-07-06 17:15:28]

Not sure about the svc.cc

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/encode.c
File core/arch/arm/encode.c (left):

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/encode.c#oldcod...
core/arch/arm/encode.c:452: if (instr == instr_get_prev(state->instr)) {
Right, this was ok when only used by DR, but once I exposed it via
dr_insert_it_instrs() we have to handle labels.

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/encode.c
File core/arch/arm/encode.c (right):

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/encode.c#newcod...
core/arch/arm/encode.c:456: ; /* do nothing to skip the label instr */
Maybe we should have instr_get_prev_nonlabel()

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/mangle.c
File core/arch/arm/mangle.c (right):

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/mangle.c#newcod...
core/arch/arm/mangle.c:543: /* A lable instruction may be used as a cti target,
so we stop
spelling

https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c
File core/arch/interp.c (right):

https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c#newcode4253
core/arch/interp.c:4253: * encode state to avoid holding a dangling pointer.
But we delete a lot of instrs during mangling: what's special about svc.cc?  If
there is some problem with deleting instrs, we need a general soln, not just one
for svc, right?

[zhaoqin, 2015-07-06 19:31:05]

Commit log for latest patchset:
---------------
i#1501 Port DynamoRIO to ARM: adjust label instr handling within IT blocks

- add encode_reset_it_block to reset encode state when necessary
- reset encode state in build_bb_ilist when adding exit instr for svc.cc
- reset encode state before emitting a fragment.
- skip label instrunction in encode_in_it_block getting prev instr
- stop IT block construction on label instruction

Review-URL: https://codereview.appspot.com/252940043
---------------

[zhaoqin, 2015-07-06 19:31:07]

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/encode.c
File core/arch/arm/encode.c (right):

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/encode.c#newcod...
core/arch/arm/encode.c:456: ; /* do nothing to skip the label instr */
On 2015/07/06 17:15:28, bruening wrote:
> Maybe we should have instr_get_prev_nonlabel()

also instr_get_next_nonlable(), not sure about that, only 1 or 2 places used
that.
maybe in a separate CL.

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/mangle.c
File core/arch/arm/mangle.c (right):

https://codereview.appspot.com/252940043/diff/1/core/arch/arm/mangle.c#newcod...
core/arch/arm/mangle.c:543: /* A lable instruction may be used as a cti target,
so we stop
On 2015/07/06 17:15:28, bruening wrote:
> spelling

Done.

https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c
File core/arch/interp.c (right):

https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c#newcode4253
core/arch/interp.c:4253: * encode state to avoid holding a dangling pointer.
On 2015/07/06 17:15:28, bruening wrote:
> But we delete a lot of instrs during mangling: what's special about svc.cc? 
If
> there is some problem with deleting instrs, we need a general soln, not just
one
> for svc, right?

This happens on the conditional svc within an IT block, where the encode state
keeps the instruction pointer of svc, and svc.cc will be deleted by
mangle_syscall.
So later when calling encode_in_it_block, the new code we added for skipping
label instr on getting prev instr will cause a seg fault.

This is generally a problem, as some instruction in an IT block maybe be stored
in encode state will be deleted.
The general solution would be at beginning or end of the instruction list
traversal on encoding, we should first reset the the encode state.

[bruening, 2015-07-07 21:59:06]

https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c
File core/arch/interp.c (right):

https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c#newcode4253
core/arch/interp.c:4253: * encode state to avoid holding a dangling pointer.
On 2015/07/06 19:31:06, zhaoqin wrote:
> On 2015/07/06 17:15:28, bruening wrote:
> > But we delete a lot of instrs during mangling: what's special about svc.cc? 
> If
> > there is some problem with deleting instrs, we need a general soln, not just
> one
> > for svc, right?
> 
> This happens on the conditional svc within an IT block, where the encode state
> keeps the instruction pointer of svc, and svc.cc will be deleted by
> mangle_syscall.
> So later when calling encode_in_it_block, the new code we added for skipping
> label instr on getting prev instr will cause a seg fault.

I don't see any label-handling code there: I see my old code that calls
instr_get_prev(state->instr).

Can you provide a call stack?  I don't understand the scopes and lifetimes of
what's going on.

> This is generally a problem, as some instruction in an IT block maybe be
stored
> in encode state will be deleted.
> The general solution would be at beginning or end of the instruction list
> traversal on encoding, we should first reset the the encode state.

It seems we should solve the general problem now.

[zhaoqin, 2015-07-07 22:52:26]

On 2015/07/07 21:59:06, bruening wrote:
> https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c
> File core/arch/interp.c (right):
> 
> https://codereview.appspot.com/252940043/diff/1/core/arch/interp.c#newcode4253
> core/arch/interp.c:4253: * encode state to avoid holding a dangling pointer.
> On 2015/07/06 19:31:06, zhaoqin wrote:
> > On 2015/07/06 17:15:28, bruening wrote:
> > > But we delete a lot of instrs during mangling: what's special about
svc.cc? 
> > If
> > > there is some problem with deleting instrs, we need a general soln, not
just
> > one
> > > for svc, right?
> > 
> > This happens on the conditional svc within an IT block, where the encode
state
> > keeps the instruction pointer of svc, and svc.cc will be deleted by
> > mangle_syscall.
> > So later when calling encode_in_it_block, the new code we added for skipping
> > label instr on getting prev instr will cause a seg fault.
> 
> I don't see any label-handling code there: I see my old code that calls
> instr_get_prev(state->instr).
> 
> Can you provide a call stack?  I don't understand the scopes and lifetimes of
> what's going on.
It is the code in
https://codereview.appspot.com/252940043/diff/20001/core/arch/arm/encode.c
where I replace the instr_get_prev with a loop to skip label instr.
Instead of: instr == instr_get_prev(state->instr)
The loop did: prev  = instr_get_prev(state->instr) and instr_is_label(prev).
Because state->instr is freed, prev is 0xcdcdcdcd instead, and instr_is_label
cause a sigfault.

> 
> > This is generally a problem, as some instruction in an IT block maybe be
> stored
> > in encode state will be deleted.
> > The general solution would be at beginning or end of the instruction list
> > traversal on encoding, we should first reset the the encode state.
> 
> It seems we should solve the general problem now.
It is kind of general, it happens if the IT block just finish and encode_state
remembers the last instr, and that instruction is removed for some reason, e.g.,
mangle_syscall. Then state->instr now is a dangling pointer.

[zhaoqin, 2015-07-08 01:22:05]

>
>  I don't see any label-handling code there: I see my old code that
>>
> calls
>
>> instr_get_prev(state->instr).
>>
>
>  Can you provide a call stack?  I don't understand the scopes and
>>
> lifetimes of
>
>> what's going on.
>>
> It is the code in
> https://codereview.appspot.com/252940043/diff/20001/core/arch/arm/encode.c
> where I replace the instr_get_prev with a loop to skip label instr.
> Instead of: instr == instr_get_prev(state->instr)
> The loop did: prev  = instr_get_prev(state->instr) and
> instr_is_label(prev).
> Because state->instr is freed, prev is 0xcdcdcdcd instead, and
> instr_is_label cause a sigfault.
>
> This happens in emit_fragment, and the stale instr is added before mangle
in the encode/decode consistency check.

>
>  > This is generally a problem, as some instruction in an IT block
>>
> maybe be
>
>> stored
>> > in encode state will be deleted.
>> > The general solution would be at beginning or end of the instruction
>>
> list
>
>> > traversal on encoding, we should first reset the the encode state.
>>
>
>  It seems we should solve the general problem now.
>>
> It is kind of general, it happens if the IT block just finish and
> encode_state remembers the last instr, and that instruction is removed
> for some reason, e.g., mangle_syscall. Then state->instr now is a
> dangling pointer.
>
>
> https://codereview.appspot.com/252940043/
>

[zhaoqin, 2015-07-08 01:39:29]

> > It is kind of general, it happens if the IT block just finish and
> > encode_state remembers the last instr, and that instruction is removed
> > for some reason, e.g., mangle_syscall. Then state->instr now is a
> > dangling pointer.
> >
but I am not sure if there is any general way to prevent this.
I added a reset function, and call it at the beginning of emit_fragment.

[bruening, 2015-07-09 02:48:29]

On 2015/07/08 01:39:29, zhaoqin wrote:
> > > It is kind of general, it happens if the IT block just finish and
> > > encode_state remembers the last instr, and that instruction is removed
> > > for some reason, e.g., mangle_syscall. Then state->instr now is a
> > > dangling pointer.
> > >
> but I am not sure if there is any general way to prevent this.
> I added a reset function, and call it at the beginning of emit_fragment.

So even if the code that de-references instr were removed, it could still
incorrectly match a re-used heap address if an instr were freed and re-created.

Some ideas:

1) Revisit from square one the encoding of IT blocks: IIRC we talked about only
supporting for whole instrlists in the past and other things.  I don't think any
of those other solutions were very good but we could at least give them another
look.

2) Have instr_destroy() clear the encode state instr for the cur dc: though that
only works for per-thread-heap instr_t (unless we want to pay the cost of a
get-dc)

3) Instead of storing the instr_t*, store the PC of...err...ok just 2 ideas.

[zhaoqin, 2015-07-09 02:56:34]

On 2015/07/09 02:48:29, bruening wrote:
> On 2015/07/08 01:39:29, zhaoqin wrote:
> > > > It is kind of general, it happens if the IT block just finish and
> > > > encode_state remembers the last instr, and that instruction is removed
> > > > for some reason, e.g., mangle_syscall. Then state->instr now is a
> > > > dangling pointer.
> > > >
> > but I am not sure if there is any general way to prevent this.
> > I added a reset function, and call it at the beginning of emit_fragment.
> 
> So even if the code that de-references instr were removed, it could still
> incorrectly match a re-used heap address if an instr were freed and
re-created.
> 
> Some ideas:
> 
> 1) Revisit from square one the encoding of IT blocks: IIRC we talked about
only
> supporting for whole instrlists in the past and other things.  I don't think
any
> of those other solutions were very good but we could at least give them
another
> look.

I think the key is to enforce the state reset, but I am not sure how.


> 
> 2) Have instr_destroy() clear the encode state instr for the cur dc: though
that
> only works for per-thread-heap instr_t (unless we want to pay the cost of a
> get-dc)
This one would have the overhead of get/set encode_state on every call, I am not
sure if you would accept the overhead.

> 
> 3) Instead of storing the instr_t*, store the PC of...err...ok just 2 ideas.

[bruening, 2015-07-10 21:24:48]

Well I wish we had a more general soln.  You mentioned a reset at top of
instrlist_encode(), any reason not to add that now in this CL?  Can you add some
comments by the 2 resets you added in this CL (svc.cc and in emit) w/ XXX for
more general soln?  Maybe file a separate issue on it so we can track this
dangling pointer issue.  Since this CL makes some progress LGTM but let's file
an issue since it's not fully solved.

[zhaoqin, 2015-07-10 22:04:48]

Commit log for latest patchset:
---------------
i#1501 Port DynamoRIO to ARM: adjust label instr handling within IT blocks

- add encode_reset_it_block to reset encode state when necessary
- reset encode state in build_bb_ilist when adding exit instr for svc.cc
- reset encode state before emitting a fragment.
- skip label instrunction in encode_in_it_block getting prev instr
- stop IT block construction on label instruction

Review-URL: https://codereview.appspot.com/252940043
---------------

[zhaoqin, 2015-07-10 22:14:23]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/9b0cee06e9872b4f3881908615f0b4c...

Final commit log: 
---------------
i#1501 Port DynamoRIO to ARM: adjust label instr handling within IT blocks

- add encode_reset_it_block to reset encode state when necessary
- reset encode state in build_bb_ilist when adding exit instr for svc.cc
- reset encode state before emitting a fragment.
- skip label instrunction in encode_in_it_block getting prev instr
- stop IT block construction on label instruction

Review-URL: https://codereview.appspot.com/252940043
---------------