Closed Bug 1005658 Opened 10 years ago Closed 10 years ago

Don't pass stack pointers to the GL for buffers, and have GLContext try to guard against it


(Core :: Graphics, defect)

Other Branch
Not set





(Reporter: bjacob, Assigned: bjacob)




(2 files)

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: nobody → bjacob
Oh and I mean, GL entry points taking buffers, such as glBufferData, glReadPixels, glTexImage2D.

It's OK to pass stack pointers to glGetIntegerv.
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
OS: Linux → All
Hardware: x86_64 → All
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)
Attachment #8417085 - Flags: review?(jgilbert) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1012735
You need to log in before you can comment on or make changes to this bug.