CAGLTexture: heap-allocate the pixel buffer in -_writeToPNG: (stack overflow)#11
CAGLTexture: heap-allocate the pixel buffer in -_writeToPNG: (stack overflow)#11DTW-Thalion wants to merge 3 commits into
Conversation
-_writeToPNG: read the texture into a stack VLA, char pixels[[self width]*[self height]*4]. For a 1024x1024 texture that is 4 MB on the stack -- a stack overflow. Allocate it on the heap and free it.
ivucica
left a comment
ivucica
left a comment
There was a problem hiding this comment.
@DTW-Thalion These three PRs are an interesting set of patches. Thank you!
Are you working on a project that will (finally) use the library? I'd be curious to learn more about it. I had reasons why I wanted a (~correctly implemented and opensource) version of CA, but I never got far enough to get that beyond the idea stage.
I'm also curious if you have any larger improvements in the pipeline that might warrant a design discussion? Example: I never got around to working on CATransition, and shadow radiuses are unlikely to be correct, and I don't think there are background or contents filters implemented in any way (likely an incompatible hack makes sense rather than full interface compatibility for those filters, as IIRC they're done via CI filters which I don't think GNUstep has an implementation of).
I have only one real request to change, just move free() earlier in the function so anyone adding an early return has a smaller chance of creating a leak. (Even if it is a debug method.)
It's also optional; if you'd prefer not to do it, I won't block this.
| CGImageRelease(image); | ||
|
|
||
| [data writeToFile:path atomically:YES]; | ||
| free(pixels); |
There was a problem hiding this comment.
Please free this as soon as it is no longer necessary, after line 264.
There was a problem hiding this comment.
Change made based on a bit of extra digging.
There was a problem hiding this comment.
d46b153 is addressing this but can't you do it straight after line 264? Immediately after CGBitmapContextCreate? Again, we can merge as is (I'm needlessly nitpicky) but doing it asap = less chance of leaks. (Less important in this debug function, but still nice)
There was a problem hiding this comment.
(If it causes a crash due to CGBitmapContextCreate not actually copying the pixels array, that's ok, then existing state is correct. I can merge this anyway.)
There was a problem hiding this comment.
That is what I am seeing - it needs to be after the CGImageRelease(image);
| - (void) _writeToPNG:(NSString*)path | ||
| { | ||
| char pixels[[self width]*[self height]*4]; | ||
| char *pixels = malloc([self width]*[self height]*4); |
There was a problem hiding this comment.
This is a nice and correct improvement.
But, please note this is an internal debug method, and should not really be used in any production code. I hope you are not seeing this anywhere in particular except while developing. This method is not part of the public API (in fact, this entire class is not part of it, and we should probably rename that and a bunch of other things). The correct way to save the image is not to query OpenGL for the pixels; it is to either use CARenderer and save the whole scene, or to get contents via CGBitmapContextRef and save that (luckily, we don't yet have anything other than CGBitmapContextRef as source of contents). Downside of using contents of a CALayer is, of course, you don't get the exact pixels fed into pixelshaders, so it's not as easy to get intermediary GL textures if you ever need that.
And a small remark: By itself, 8MB default stack size on Linux and macOS should typically be fine for your example of 4MB. Apparently Windows has 1MB limit; if you're porting this to Windows, this would be very exciting to me, I think I never built a CA-using binary on Windows.
There was a problem hiding this comment.
This is something our folks are trying to decide internally. For our needs we have a lot of rendering happening - and we're trying to figure out how we want our researcher workstations to operate. We may end up using Unreal Engine instead, but we wanted to play with what exists already in the GNUStep world.
Move free(pixels) to immediately after CGImageRelease(image), before the file is written, so a future early return added around the file write cannot leak the buffer (per review feedback). The buffer cannot be freed any earlier: CGBitmapContextCreate uses it in place as the bitmap backing store and CGBitmapContextCreateImage returns an image that references it through a data provider (no snapshot), so it must stay live until CGImageRelease.
|
@ivucica to answer your question on our project: we are building a large computational biology engine with modelling of cells and proteins (https://thalion.global/programs/computational-modelling). In April we made a decision to consider Objective-C as the language of choice for our server architecture, and that precipitated doing a review of how solid GNUStep would be in a high-availability, multi-node, multi-threaded environment. So we did a first pass to scope the various repos using Claude 4.6 (which is how PR#6/7 happened). With that in hand, we took a copy of the repos and had our dev-team start building test harnesses for our usage model (multiple-GPUs, multi-node, etc.) and did a lot of load testing in various configurations. As we did that we would encounter various issues, and they would be reported internally. When we find an issue, we fix it on our side, but to offer it back to the community, we have to unbolt it from our codebase, and when I have time I have been picking away at packaging up each as a seperate PR in the form that RFM asked for. So that is what you are seeing now. I had offered a number of changes directly from the Claude review, but noted the pushback on anything LLM direct, so that's fair, I am just much slower than LLM and work on them as I have time and build out the PRs by hand as necessary. |
Add Tests/headless, a display-free test that creates a surfaceless EGL
desktop-GL context (Mesa software rendering) and exercises
CAGLTexture -_writeToPNG: directly, obtaining the class with objc_getClass:
- a texture round-trips to a valid PNG of the expected dimensions;
- writing a large texture does not overflow the stack -- the previous
implementation read the image into a stack VLA (char pixels[w*h*4]).
The large-texture case runs on a thread with a deliberately small stack so
the regression is deterministic regardless of the caller's stack limit.
The test only builds when EGL is available (pkg-config egl) and skips at
run time if no GL context can be created, so it is inert where headless GL
is unavailable. Run it with "make check" in Tests/headless.
|
Added It creates a surfaceless EGL context backed by Mesa's software rasteriser (llvmpipe), obtains The target is gated on EGL: it only builds when The one CI cost is the dependency: to actually run it the runner needs EGL plus a Mesa software-GL driver. On Ubuntu that's Since you mentioned headless GL testing never got past the idea stage — this approach (surfaceless EGL + |
2 participants
Summary
-_writeToPNG:reads the whole texture into a stack VLA:For a 1024×1024 texture that is 4 MB on the stack — a stack overflow.
Fix
Allocate the buffer on the heap (with a NULL check) and
freeit at the end.The buffer is used as the backing store for the bitmap context until
CGBitmapContextCreateImagesnapshots it, so it is freed only after the imagehas been created and written.
Validation
Built
libQuartzCore(clang, gnustep-2.0, against libs-corebase + libs-opal +libs-gui);
CAGLTexture.mcompiles cleanly.