Skip to content

Commit 4d2d997

Browse files
committed
gfx/drivers/metal: NULL-check _viewport and video_shader callocs in init and shader load paths
Two unchecked C allocs in metal.m that NULL-deref downstream on OOM. The Metal driver is predominantly ARC-managed Objective-C, but these two heap-allocated C structs survive outside the ARC object graph and had no NULL handling. === MetalDriver initWithVideo: _viewport calloc === The MetalDriver class exposes a 'viewport' property whose backing ivar is a video_viewport_t* allocated at init time: @Property (nonatomic, readwrite) video_viewport_t *viewport; ... _viewport = (video_viewport_t *)calloc(1, sizeof(video_viewport_t)); _viewportMVP.projectionMatrix = matrix_proj_ortho(0, 1, 0, 1); The calloc was unchecked. Two later paths dereference the pointer: 1. Context's setViewport: handler (Context is the MetalDriver's _context ivar) does _viewport = *viewport; - unconditional dereference. MetalDriver assigns to _context.viewport via the property setter in setVideoMode / show_mouse / frame, which would crash on a NULL _viewport. 2. FrameView also holds a video_viewport_t *_viewport ivar, assigned from MetalDriver's _viewport via _frameView.viewport = _viewport. FrameView reads _viewport->x/y/width/height unconditionally in its render path. Fix: NULL-check the calloc and 'return nil' from the init method. This matches the bailout pattern used by the _initMetal failure branch just above. ARC will dealloc the partial instance - MetalDriver's dealloc is already NULL-safe for _viewport (checks 'if (_viewport) free(_viewport);') and for _gpu_list (checks 'if (_gpu_list)'). === setShaderFromPath: shader calloc === struct video_shader *shader = (struct video_shader *)calloc(1, sizeof(*shader)); settings_t *settings = config_get_ptr(); ... @Try { if (!video_shader_load_preset_into_shader(path.UTF8String, shader)) return NO; ... } @finally { if (shader) [self _freeVideoShader:shader]; } The calloc was unchecked. video_shader_load_preset_into_shader in gfx/video_shader_parse.c receives shader as its second parameter and writes shader->path / shader->passes / etc. with no NULL-guard on the pointer - so a NULL shader would crash inside the shader parser. Fix: NULL-check before entering the @Try and 'return NO' immediately. The caller treats this as a shader-load failure and falls back to the stock (non-slang) render path. The @finally cleanup block is a no-op for NULL shader via the guard in _freeVideoShader, so no cleanup concern. === Swept-clean in the same pass === Verified NULL-checked in metal.m: - heapRow malloc in readViewport:: NULL-checked with early 'return NO;'. - No other raw malloc/calloc/realloc sites in gfx/drivers/metal.m or in gfx/common/metal/*. The rest of the driver is ARC-managed Objective-C with Metal framework object lifetimes; out-of-scope for this audit pass. === Note on _inflightSemaphore === The MetalDriver Context class declares a dispatch_semaphore_t _inflightSemaphore and allocates it via dispatch_semaphore_create(MAX_INFLIGHT). The declaration and creation are the only two references in the file - the semaphore is never actually wait()ed or signal()ed. It's effectively dead code, so no OOM handling is needed; left alone for this audit. Scope: local to metal.m. No API changes, no header changes, no changes to other .m files under gfx/common/metal/. Thread-safety: both fixes are in one-shot paths called from the main video thread at driver creation / shader load time; no concurrency concerns. Reachability: both are reachable on OOM. The _viewport one is unconditional at driver creation - every Metal driver instance hits it. The shader one fires whenever a slang preset is loaded (menu selection, CLI --set-shader, remote control), which is a direct user action.
1 parent d44ae5e commit 4d2d997

1 file changed

Lines changed: 21 additions & 0 deletions

File tree

gfx/drivers/metal.m

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3378,6 +3378,17 @@ - (instancetype)initWithVideo:(const video_info_t *)video
33783378

33793379
_video = *video;
33803380
_viewport = (video_viewport_t *)calloc(1, sizeof(video_viewport_t));
3381+
/* NULL-check: Context's setViewport: (see line ~516)
3382+
* unconditionally dereferences the pointer via
3383+
* _viewport = *viewport; A NULL here would crash on
3384+
* the first _context.viewport = _viewport assignment
3385+
* downstream in setVideoMode / show_mouse / frame.
3386+
* Return nil to fail the init - matches the pattern
3387+
* used by the _initMetal bailout just above. ARC will
3388+
* tear down the partial instance (Metal library/queue
3389+
* ref-counts, string_list _gpu_list) via dealloc. */
3390+
if (!_viewport)
3391+
return nil;
33813392
_viewportMVP.projectionMatrix = matrix_proj_ortho(0, 1, 0, 1);
33823393

33833394
_keepAspect = _video.force_aspect;
@@ -4590,6 +4601,16 @@ - (BOOL)setShaderFromPath:(NSString *)path
45904601
_shaderEmitsHDR16 = NO;
45914602

45924603
struct video_shader *shader = (struct video_shader *)calloc(1, sizeof(*shader));
4604+
/* NULL-check: video_shader_load_preset_into_shader below
4605+
* writes shader->path / shader->passes etc. via field access
4606+
* with no NULL guard on its second parameter, so a NULL
4607+
* shader would crash inside the parser. Return NO here -
4608+
* caller treats this as a shader-load failure and falls
4609+
* back to the stock (non-slang) render path. The @finally
4610+
* block below is a no-op for NULL shader via the guard in
4611+
* _freeVideoShader. */
4612+
if (!shader)
4613+
return NO;
45934614
settings_t *settings = config_get_ptr();
45944615
const char *dir_video_shader = settings->paths.directory_video_shader;
45954616
NSString *shadersPath = [NSString stringWithFormat:@"%s/", dir_video_shader];

0 commit comments

Comments
 (0)