Closed Bug 422825 Opened 16 years ago Closed 16 years ago

uninitialized memory-read in XPCWrapper::AddProperty

Categories

(Core :: XPConnect, defect, P1)

Product:
Core
Component:
XPConnect
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta5

People

(Reporter: crowderbt, Assigned: crowderbt)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file)

init v
932 bytes, patch
crowderbt
: review+
crowderbt
: superreview+
ted
: approval1.9+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #420585 +++

The if (!isXOW) sequence in XPCWrapper::AddProperty() seems to allow a situation where |v| is used in OBJ_DEFINE_PROPERTY uninitialized.  I am hitting this case while using firebug 1.2 with a recent nightly build.

This patch may be wrong.  Shouldn't we init as *vp?  mrbkap suggested JSVAL_VOID on IRC, and I have used that to continue my work, but I'm not sure what is more correct.

I'm also not sure how to architect a testcase that hits this issue.  Filing as security-sensitive, for now, though I'm very sure this is a trunk-only (and recent, at that) bug.
Woops.  Lost my summary in a minefield crash.
Summary: Defining setters for properties of the global object doesn't work → uninitialized memory-read in XPCWrapper::AddProperty
Assignee: nobody → crowder
Flags: blocking1.9?
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta5
Blocks: 420585
No longer depends on: 420585
Ah, I forgot we had a *vp coming in there. r+sr=me on using it as an initial value.
Attached patch init v — — Splinter Review 
This has r/sr from mrbkap and needs a+
Attachment #309415 - Flags: superreview+
Attachment #309415 - Flags: review+
Attachment #309415 - Flags: approval1.9?
Comment on attachment 309415 [details] [diff] [review]
init v

a=me for crash fix
Attachment #309415 - Flags: approval1.9? → approval1.9+
Thanks, Ted.  If I'm not around when the tree reopens, I'm hoping someone will land.  Killing sec-sensitive flag on this UMR bug, since it is trunk-only, recent, and soon-to-be-patched.
Group: security
Keywords: checkin-needed
XPCWrapper.cpp:1.25
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
This deserves a testcase, but I confess I'm not sure how to concoct one that excercises this line of code; I know only that firebug hits it.
Flags: in-testsuite?
Clearing blocking, since this has a+ and has landed.
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: