Closed Bug 655390 Opened 9 years ago Closed 9 years ago

argcOk should probably do a runtime check for argc<0

Categories

(Tamarin Graveyard :: Virtual Machine, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Q4 11 - Anza

People

(Reporter: stejohns, Assigned: stejohns)

Details

Attachments

(1 file, 1 obsolete file)

Currently argcOk merely asserts that argc >= 0, but there have been code paths in the past that allowed breaking this invariant, and some may still exist. Since argc < 0 is *never* ok, and the test is cheap, let's always do it.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → stejohns
Attachment #530756 - Flags: superreview?(edwsmith)
Attachment #530756 - Flags: review?(rreitmai)
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Priority: -- → P3
Target Milestone: --- → Q4 11 - Anza
If you do the upper bound check with an unsigned comparison then you probably don't need the explicit >= 0 check:

(argc <= _param_count || _allowExtraArgs)

right?
only if we change it to uint32_t(argc) <= uint32_t(_param_count), I think.
Attached patch Patch v2Splinter Review
Attachment #530756 - Attachment is obsolete: true
Attachment #530756 - Flags: superreview?(edwsmith)
Attachment #530756 - Flags: review?(rreitmai)
Attachment #532705 - Flags: superreview?(edwsmith)
Attachment #532705 - Flags: review?(rreitmai)
Comment on attachment 532705 [details] [diff] [review]
Patch v2

Haven't looked deeply but I'm assuming that we've guaranteed that _param_count is > 0 ( Should we assert on it? )

If not, then I'd prefer spelling out the checks and letting the compiler do whatever reduction can be done.  I.e. Something like 'argc >= 0 && _param_count >= 0 && ... '
Attachment #532705 - Flags: review?(rreitmai) → review+
Attachment #532705 - Flags: superreview?(edwsmith) → superreview+
TR 6359:206038c93867
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.