OP_bkpt seen in the wild

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P2
normal
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Edwin Smith, Assigned: Krzysztof Palacz)

Tracking

unspecified
flash10.1
Bug Flags:
in-testsuite +
flashplayer-qrb +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
OP_bkpt (0x01) was thought to have never been used, and removed, but is actually present on the web in at least one case.  We need to restore it and support it, and maybe deprecate it in newer ABC versions.
(Reporter)

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → flash10.1

Comment 1

9 years ago
Foo :-)

Do we know the tool that produced this code?
(Reporter)

Comment 2

9 years ago
judging from style, i'd say it's from ASC.  if the debugger mutates abc to add OP_bkpt instructions, then it might not be ASC's fault.
(Reporter)

Comment 3

9 years ago
The thing is, we should be getting a VerifyError about an invalid opcode, but instead we're getting an assert.  But we get lucky: with the assert compiled out, we do what we should do: skip ahead one byte to the next opcode.  If we support OP_bkpt, we should skip over it without the assert.  If we stop supporting it for real, we should throw the VerifyError.

Updated

9 years ago
Flags: flashplayer-qrb+
(Reporter)

Comment 4

9 years ago
OP_0xF2 (used to be bkptline) and 0xF3 (timestamp) are also in a similar situation.  their size is not -1, yet they are not handled by the Verifier and go to the default case and assert.

Updated

9 years ago
Assignee: nobody → kpalacz

Updated

9 years ago
Status: NEW → ASSIGNED

Comment 5

9 years ago
These 3 opcodes will not be supported in Argo and should be fully neutered.  The debug builds should not generate asserts() and the  opcodes should be converted to NOPs.  Please include test cases for each opcode to show it is ignored.
(Assignee)

Comment 6

9 years ago
Created attachment 427894 [details] [diff] [review]
Uniform handling of bkpt, bkptline and timestamp: recognize but otherwise ignore

I have testcases (I updated the tools to make them), but will have to figure out if and how to integrate with the acceptance suite.
Formatting note: some of the files use tabs, my additions use spaces, it may look weird, I suggest that we reformat later in a reformat-only patch.
Attachment #427894 - Flags: review?(lhansen)
(Assignee)

Comment 7

9 years ago
Created attachment 429860 [details] [diff] [review]
adds test cases
Attachment #427894 - Attachment is obsolete: true
Attachment #429860 - Flags: review?(lhansen)
Attachment #427894 - Flags: review?(lhansen)

Comment 8

9 years ago
I'm not sure where the test cases are here.
(Assignee)

Comment 9

9 years ago
Created attachment 430176 [details] [diff] [review]
adds test cases (for real this time).
Attachment #429860 - Attachment is obsolete: true
Attachment #430176 - Flags: review?(lhansen)
Attachment #429860 - Flags: review?(lhansen)

Updated

9 years ago
Attachment #430176 - Flags: review?(lhansen) → review+

Comment 10

9 years ago
tr-argo changeset 3789:c9a0d821de24
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

8 years ago
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.