Skip to content

fix(FUN-6): NULL-check malloc at three sites to prevent NULL-deref crashes#238

Open
kostub wants to merge 1 commit into
masterfrom
em/2026-06-11-issues/t14
Open

fix(FUN-6): NULL-check malloc at three sites to prevent NULL-deref crashes#238
kostub wants to merge 1 commit into
masterfrom
em/2026-06-11-issues/t14

Conversation

@kostub

@kostub kostub commented Jun 12, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes FUN-6 (issues.md#L233) — three malloc calls wrote to the returned buffer immediately without a NULL check.

  • MTMathListBuilder.m (parser, realistic risk): initWithString: now guards malloc(sizeof(unichar)*_length). On NULL, sets _length = 0 and skips getCharacters:, degrading to an empty MTMathList instead of NULL-dereferencing. dealloc already does free(_chars); free(NULL) is a safe no-op.
  • MTMathListDisplay.mMTGlyphConstructionDisplay (low-risk, hardened for consistency): guards _glyphs/_positions allocations; on failure sets _numGlyphs = 0 and frees any partial allocation so draw: becomes a CTFontDrawGlyphs(..., 0, ...) no-op.
  • MTMathListDisplay.mMTHorizontalGlyphAssemblyDisplay (same pattern): identical guard applied to the second glyph-assembly initializer.

Net change: ~+16 LOC across 2 files, no API or architecture change.

Test plan

  • swift build — clean build, no warnings introduced
  • swift test — 293 tests, 0 failures
  • New test testEmptyInputParsesToEmptyList added to MTMathListBuilderTest — parses @"", asserts zero atoms and no error, locking in the "degrade to empty" contract that the malloc-failure fallback also produces
  • malloc-returns-NULL branches verified by code inspection (deterministically forcing malloc failure in XCTest requires a malloc interposer and is platform-fragile; documented in solution.md)

🤖 Generated with Claude Code

…ashes

- MTMathListBuilder initWithString: guards malloc(_length*sizeof(unichar))
  and degrades to empty input (instead of dereferencing NULL) on failure
- MTGlyphConstructionDisplay and MTHorizontalGlyphAssemblyDisplay guard
  their glyph/position allocations; set _numGlyphs=0 on failure so draw:
  becomes a safe no-op (CTFontDrawGlyphs with count 0)
- Add testEmptyInputParsesToEmptyList to lock in the empty-input contract
  (same terminal state as the parser OOM fallback); malloc-NULL branches
  are verified by inspection per solution.md

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces safety checks for memory allocation failures and empty inputs in MTMathListBuilder and MTMathListDisplay, along with a new unit test for empty input parsing. The feedback suggests optimizing the memory allocation logic in MTMathListDisplay by wrapping the malloc calls in a check to ensure _numGlyphs is greater than zero, which avoids redundant allocations and potential implementation-defined behavior when the glyph count is zero.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 614 to +620
_glyphs = malloc(sizeof(CGGlyph) * _numGlyphs);
_positions = malloc(sizeof(CGPoint) * _numGlyphs);
if (_numGlyphs > 0 && (!_glyphs || !_positions)) {
free(_glyphs); _glyphs = NULL;
free(_positions); _positions = NULL;
_numGlyphs = 0; // nothing to draw; CTFontDrawGlyphs is a no-op for count 0
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling malloc when _numGlyphs is 0 is unnecessary and can be avoided entirely. malloc(0) has implementation-defined behavior (it may return NULL or a non-NULL pointer that should not be dereferenced but must be freed). Wrapping the allocations in a _numGlyphs > 0 check avoids these redundant calls and potential edge cases.

        if (_numGlyphs > 0) {
            _glyphs = malloc(sizeof(CGGlyph) * _numGlyphs);
            _positions = malloc(sizeof(CGPoint) * _numGlyphs);
            if (!_glyphs || !_positions) {
                free(_glyphs); _glyphs = NULL;
                free(_positions); _positions = NULL;
                _numGlyphs = 0;
            }
        }

Comment on lines 958 to +964
_glyphs = malloc(sizeof(CGGlyph) * _numGlyphs);
_positions = malloc(sizeof(CGPoint) * _numGlyphs);
if (_numGlyphs > 0 && (!_glyphs || !_positions)) {
free(_glyphs); _glyphs = NULL;
free(_positions); _positions = NULL;
_numGlyphs = 0; // nothing to draw; CTFontDrawGlyphs is a no-op for count 0
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling malloc when _numGlyphs is 0 is unnecessary and can be avoided entirely. malloc(0) has implementation-defined behavior (it may return NULL or a non-NULL pointer that should not be dereferenced but must be freed). Wrapping the allocations in a _numGlyphs > 0 check avoids these redundant calls and potential edge cases.

        if (_numGlyphs > 0) {
            _glyphs = malloc(sizeof(CGGlyph) * _numGlyphs);
            _positions = malloc(sizeof(CGPoint) * _numGlyphs);
            if (!_glyphs || !_positions) {
                free(_glyphs); _glyphs = NULL;
                free(_positions); _positions = NULL;
                _numGlyphs = 0;
            }
        }

@kostub

kostub commented Jun 12, 2026

Copy link
Copy Markdown
Owner Author

EM-REVIEW v1

Code Review: FUN-6 NULL-check malloc at three sites.

Scope reviewed: full diff plus surrounding source in MTMathListBuilder.m (initWithString:, dealloc, getNextCharacter) and both glyph-assembly initializers in MTMathListDisplay.m.

Verdict: No blocking issues. The change is correct, minimal, and consistent with existing patterns.

Correctness:

  1. MTMathListBuilder.m initWithString: — Correct. The _length > 0 + NULL guard is sound. When _length == 0, malloc is skipped and _chars stays NULL; dealloc does free(NULL) (no-op), and getNextCharacter/hasCharacters gate on _currentChar < _length which is never true at length 0, so no NULL deref is reachable. Behavior vs old code: previously malloc(0) ran for empty strings (implementation-defined return); skipping it is strictly safer and observably identical. On malloc failure with non-empty input, degrading to _length = 0 (empty list, no error) is reasonable fail-soft for OOM.
  2. MTMathListDisplay.m both initWithGlyphs: initializers — Correct. _numGlyphs is NSInteger (lines 603 and 941), so _numGlyphs > 0 is a clean signed comparison. The guard correctly handles partial-failure (one malloc succeeds, other fails) by freeing both and zeroing the count, so the for-loop and CTFontDrawGlyphs(..., 0, ...) in draw: both become safe no-ops. The _numGlyphs > 0 precondition also avoids treating a legitimate malloc(0)==NULL (empty-glyph case) as a failure.

Minor / non-blocking:

  • Two near-identical guard blocks in MTMathListDisplay.m are copy-pasted. Not worth a helper for ~5 lines x2, but worth noting if a third such display is added.
  • Test coverage is indirect: testEmptyInputParsesToEmptyList exercises the _length == 0 branch but not the actual malloc-returns-NULL paths. The PR body acknowledges this and the justification (no malloc interposer) is fair. The empty-input test still locks in the degrade-to-empty contract.
  • Fail-soft under OOM silently produces empty output rather than surfacing an error. Acceptable for a rendering library (a crash would be strictly worse) and consistent across all three sites.

Summary: All three NULL-checks are correct; edge cases (length 0, partial allocation failure, free-of-NULL in dealloc) are handled properly. Tests pass per the PR description. No changes required to merge. Not approving per review protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant