Closed
Bug 1005658
Opened 9 years ago
Closed 9 years ago
Don't pass stack pointers to the GL for buffers, and have GLContext try to guard against it
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
Attachments
(2 files)
15.31 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
1.80 KB,
patch
|
Details | Diff | Splinter Review |
Most users pass heap pointers, not stack pointers to the GL. So when we pass a stack pointer, we are putting ourselves in a minority case. Moreover we then have no more alignment than what is required to store individual elements. E.g. a GLfloat vertices[] array doesn't have more than 4 byte alignment... It's not hard to imagine how that could be surprising to a GL used to heap pointers and trying to copy client-side buffers fast. The other problem with passing stack pointers to the GL is it makes it harder to reason about crashes involving the GL and stack values.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bjacob
Assignee | ||
Comment 1•9 years ago
|
||
Oh and I mean, GL entry points taking buffers, such as glBufferData, glReadPixels, glTexImage2D. It's OK to pass stack pointers to glGetIntegerv.
Assignee | ||
Updated•9 years ago
|
Summary: Don't pass stack pointers to the GL, and have GLContext try to guard against it → Don't pass stack pointers to the GL for buffers, and have GLContext try to guard against it
Assignee | ||
Updated•9 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 2•9 years ago
|
||
This patch: 1) Removes MOZ_ENABLE_GL_TRACKING and instead uses DEBUG, which is synonym of it. Reason: comments about #else / #endif statements in GLContext.h were mentioning the wrong one, and that confused me. Since MOZ_ENABLE_GL_TRACKING is defined if and only if DEBUG is, it seems like a useless duplicate anyway. 2) Adds the assertion discussed above, with just the aggressiveness required for it to catch the CompositorOGL::Initialize case on my system. 3) Adds a HeapCopyOfStackArray helper to be able to easily convert stack arrays into heap arrays, to be able to retain the convenience of plain old static arrays and not have to introduce more custom dangerous MOZ_ARRAY_LENGTH / sizeof usage. 4) Uses that in CompositorOGL::Initialize.
Attachment #8417085 -
Flags: review?(jgilbert)
Assignee | ||
Comment 3•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a83ae6f600ea
Updated•9 years ago
|
Attachment #8417085 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4f00793f49ae
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbef254f0aa5
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fbef254f0aa5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•