Closed Bug 1051989 Opened 10 years ago Closed 10 years ago

Assert against correctly aligned memory, rather than stack/heap allocation, in GLContext

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: gw280, Unassigned)

Details

After speaking with Jeff, we think it makes more sense to check for correctly aligned pointers rather than stack/heap allocations. Originally from bug 1005658.
Why?

I was specifically concerned about having buggy GL corrupt our stack, making crash reports harder to interprete. The point of preventing us from passing stack pointers to the GL was mostly to cut out an entire class of memory corruption (stack corruption) when having to interprete crash reports.
Have we ever encountered a GL driver which corrupts the stack like that? I realise that in theory it can happen, but there's a fair bit of code in skia where they use an allocator for temporary buffers that uses the stack if the allocation requested is below a certain size as stack allocation is typically much faster than heap.
Bug 1004134 is what started this conversation. This was a hard to understand crash in a correct call to glBufferData; the only thing that seemed special about it was that it was one of the very few times where we passed a stack buffer. I haven't heard back from this user so I don't know if it was affected in any way by this change.

But drivers do crazy things with the stack. On Linux at least, as of about 3 years ago at least, the NVIDIA proprietaty driver was setting (mprotect) executable permissions on some stack pages.... (see Bug 608526). So I've often found myself trying to guess whether a strange driver crash could be related to stack corruption.
(In reply to George Wright (:gw280) from comment #2)
> Have we ever encountered a GL driver which corrupts the stack like that? I
> realise that in theory it can happen, but there's a fair bit of code in skia
> where they use an allocator for temporary buffers that uses the stack if the
> allocation requested is below a certain size as stack allocation is
> typically much faster than heap.

That's a poor reason to use a stack buffer: while a heap allocation is slower than a stack allocation, it is also much faster than the GL calls that we are talking about here (glBufferData, glTexImage2D, etc).
To be clear, I don't want to block you from changing this into asserting on alignment, if you want. In fact, ideally, IMHO, we would assert on both alignment and non-stack-ness. If this is conflicting with Skia GL passing stack buffers to the GL, and changing Skia is nontrivial or nonupstreamable or for whatever reason has a real risk of regressing performance, then sure do what you need to. Just clarifying that there is a danger in passing stack pointers to the GL, as illustrated in Figure 1:



                   ____________________________________________________
                    \                                 |               |
                     \_                               |_______________|
Butcher's knife        \                              |
  = the GL              \__                           |
                           \_______                   |
                                   \__________________|

Our balls                            oo
  = the stack
                    _______________________________________________
                        |                                    |
Butcher's table         |                                    |
                        |                                    |
                        |                                    |



                         ~~~ Figure 1 ~~~
For the sake of our collective balls, I am convinced.
I got upstream to add the code to always allocate on the heap. Closing as invalid.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.