Skip to content

fix(SEC-3): make global symbol/lookup tables thread-safe #228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kostub wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(SEC-3): make global symbol/lookup tables thread-safe #228

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2e0f82
fix(SEC-3): make global symbol/lookup tables thread-safe
kostub Jun 12, 2026
d86d1bf
fix: hold gSymbolTableLock across full inner read in +latexSymbolName…
kostub Jun 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 68 additions & 34 deletions iosMath/lib/MTMathAtomFactory.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@

#import "MTMathAtomFactory.h"
#import "MTMathListBuilder.h"
#import <os/lock.h>

// Lock protecting the two genuinely-mutable symbol tables: `commands` (in
// +supportedLatexSymbols) and `textToCommands` (in +textToLatexSymbolNames).
// Read-only tables are guarded only by dispatch_once and need no run-time lock.
static os_unfair_lock gSymbolTableLock = OS_UNFAIR_LOCK_INIT;

NSString *const MTSymbolMultiplication = @"\u00D7";
NSString *const MTSymbolDivision = @"\u00F7";
Expand Down Expand Up @@ -163,9 +169,13 @@ + (nullable MTMathAtom *)atomForLatexSymbolName:(NSString *)symbolName
// Switch to the canonical name
symbolName = canonicalName;
}

NSDictionary* commands = [self supportedLatexSymbols];
MTMathAtom* atom = commands[symbolName];

// commands is a genuinely-mutable dict (addLatexSymbol: writes it), so guard.
NSMutableDictionary* commands = [self supportedLatexSymbols];
MTMathAtom* atom = nil;
os_unfair_lock_lock(&gSymbolTableLock);
atom = commands[symbolName];
os_unfair_lock_unlock(&gSymbolTableLock);
if (atom) {
// Return a copy of the atom since atoms are mutable.
return [atom copy];
Expand All @@ -178,8 +188,12 @@ + (nullable NSString*) latexSymbolNameForAtom:(MTMathAtom*) atom
if (atom.nucleus.length == 0) {
return nil;
}
NSDictionary<NSString*, NSDictionary<NSNumber*, NSString*>*>* dict = [MTMathAtomFactory textToLatexSymbolNames];
NSDictionary<NSNumber*, NSString*>* inner = dict[atom.nucleus];
// textToLatexSymbolNames is a genuinely-mutable dict (addLatexSymbol: writes it), so guard.
NSMutableDictionary* dict = [MTMathAtomFactory textToLatexSymbolNames];
NSDictionary<NSNumber*, NSString*>* inner = nil;
os_unfair_lock_lock(&gSymbolTableLock);
inner = dict[atom.nucleus];

@kostub kostub Jun 12, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

[Important — incomplete critical section / residual data race]

latexSymbolNameForAtom: releases gSymbolTableLock immediately after fetching the inner reference (inner = dict[atom.nucleus]), but then reads inner[@(atom.type)] (and the Bin-fallback inner[@(kMTMathAtomBinaryOperator)]) outside the lock.

inner is an NSMutableDictionary that the writer in +addLatexSymbol:value: mutates in place while holding the lock:

NSMutableDictionary* inner = dict[atom.nucleus];  // existing inner reused
...
inner[typeKey] = name;   // in-place mutation, under lock

So when an existing nucleus is updated by a concurrent writer, a reader holding the same inner reference reads it after unlocking — a NSMutableDictionary read concurrent with a write, i.e. the exact UB this PR is meant to close. The lock only protects the outer dict[...] lookup, not the inner dict[...] read.

Fix: keep the lock held until after name = inner[@(atom.type)] (and the fallback) are read, e.g. resolve name to a local inside the locked region and unlock just before returning. The other two readers (atomForLatexSymbolName:, supportedLatexSymbolNames) read directly under the lock and are fine; only this one unlocks too early.

Note: testConcurrentAddAndLookupSymbol’s writers insert nucleus "X" while the reader queries nucleus "→", so they touch different inner dicts and this race is never exercised. A regression test that has writers and the reverse-lookup reader share the same nucleus would expose it under TSan.

os_unfair_lock_unlock(&gSymbolTableLock);

@gemini-code-assist gemini-code-assist Bot Jun 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Although the outer dictionary dict is accessed under the lock, the retrieved inner dictionary is a mutable NSMutableDictionary that can be concurrently mutated in +addLatexSymbol:value:. Reading from inner outside the lock (on lines 200 and 208) creates a data race, which can lead to crashes or undefined behavior when LaTeX is parsed concurrently.

To resolve this safely without holding the lock during the entire lookup, you can copy the inner dictionary under the lock to obtain an immutable snapshot. Additionally, ensure you guard against nil keys (such as atom.nucleus) before accessing the dictionary to prevent NSInvalidArgumentException crashes.

    NSMutableDictionary* dict = [MTMathAtomFactory textToLatexSymbolNames];
    NSDictionary<NSNumber*, NSString*>* inner = nil;
    if (atom.nucleus != nil) {
        os_unfair_lock_lock(&gSymbolTableLock);
        inner = [dict[atom.nucleus] copy];
        os_unfair_lock_unlock(&gSymbolTableLock);
    }
References
  1. Always guard against nil keys before accessing or modifying an NSMutableDictionary (or NSDictionary) in Objective-C to prevent NSInvalidArgumentException crashes.

if (!inner) {
return nil;
}
Expand All @@ -200,10 +214,15 @@ + (void)addLatexSymbol:(NSString *)name value:(MTMathAtom *)atom
{
NSParameterAssert(name);
NSParameterAssert(atom);
// Ensure both tables are initialized before taking the lock (dispatch_once
// inside each accessor guarantees a single init; no deadlock since the tokens
// are method-local and the lock is not held during the once-block).
NSMutableDictionary<NSString*, MTMathAtom*>* commands = [self supportedLatexSymbols];
commands[name] = atom;
NSMutableDictionary<NSString*, NSMutableDictionary<NSNumber*, NSString*>*>* dict = [self textToLatexSymbolNames];

os_unfair_lock_lock(&gSymbolTableLock);
commands[name] = [atom copy]; // copy on write — symmetric with the read side
Comment on lines 217 to +226

@gemini-code-assist gemini-code-assist Bot Jun 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

In Objective-C, assertions like NSParameterAssert are disabled in release builds. If name or atom is nil at runtime in a release build, attempting to set a value in commands or copying atom will cause an NSInvalidArgumentException crash. Always guard against nil keys and values before modifying an NSMutableDictionary.

    if (!name || !atom) {
        return;
    }
    NSMutableDictionary<NSString*, MTMathAtom*>* commands = [self supportedLatexSymbols];
    NSMutableDictionary<NSString*, NSMutableDictionary<NSNumber*, NSString*>*>* dict = [self textToLatexSymbolNames];

    os_unfair_lock_lock(&gSymbolTableLock);
    commands[name] = [atom copy];
References
  1. Always guard against nil keys before accessing or modifying an NSMutableDictionary (or NSDictionary) in Objective-C to prevent NSInvalidArgumentException crashes.

if (atom.nucleus.length != 0) {
NSMutableDictionary<NSString*, NSMutableDictionary<NSNumber*, NSString*>*>* dict = [self textToLatexSymbolNames];
NSMutableDictionary<NSNumber*, NSString*>* inner = dict[atom.nucleus];
if (!inner) {
inner = [NSMutableDictionary dictionaryWithCapacity:1];
Expand All @@ -216,12 +235,16 @@ + (void)addLatexSymbol:(NSString *)name value:(MTMathAtom *)atom
inner[typeKey] = name;
}
}
os_unfair_lock_unlock(&gSymbolTableLock);
}

+ (NSArray<NSString *> *)supportedLatexSymbolNames
{
NSDictionary<NSString*, MTMathAtom*>* commands = [MTMathAtomFactory supportedLatexSymbols];
return commands.allKeys;
NSMutableDictionary<NSString*, MTMathAtom*>* commands = [MTMathAtomFactory supportedLatexSymbols];
os_unfair_lock_lock(&gSymbolTableLock);
NSArray* keys = commands.allKeys;
os_unfair_lock_unlock(&gSymbolTableLock);
return keys;
}

+ (nullable MTAccent*) accentWithName:(NSString*) accentName
Expand Down Expand Up @@ -272,7 +295,8 @@ + (MTFontStyle)fontStyleWithName:(NSString *)fontName {
+ (NSDictionary<NSString*, NSNumber*>*) textStyles
{
static NSDictionary<NSString*, NSNumber*>* textStyles = nil;
if (!textStyles) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
textStyles = @{
@"text": @(kMTTextStyleRoman),
@"textrm": @(kMTTextStyleRoman),
Expand All @@ -281,7 +305,7 @@ + (MTFontStyle)fontStyleWithName:(NSString *)fontName {
@"textsf": @(kMTTextStyleSansSerif),
@"texttt": @(kMTTextStyleTypewriter),
};
}
});
return textStyles;
}

Expand Down Expand Up @@ -367,14 +391,15 @@ + (nullable MTMathAtom *)tableWithEnvironment:(NSString *)env rows:(NSArray<NSAr
}
}
static NSDictionary<NSString*, NSArray*>* matrixEnvs = nil;
if (!matrixEnvs) {
static dispatch_once_t matrixEnvsOnce;
dispatch_once(&matrixEnvsOnce, ^{
matrixEnvs = @{ @"matrix" : @[],
@"pmatrix" : @[ @"(", @")"],
@"bmatrix" : @[ @"[", @"]"],
@"Bmatrix" : @[ @"{", @"}"],
@"vmatrix" : @[ @"vert", @"vert"],
@"Vmatrix" : @[ @"Vert", @"Vert"], };
}
});
if ([matrixEnvs objectForKey:env]) {
// it is set to matrix as the delimiters are converted to latex outside the table.
table.environment = @"matrix";
Expand Down Expand Up @@ -493,7 +518,8 @@ + (nullable MTMathAtom *)tableWithEnvironment:(NSString *)env rows:(NSArray<NSAr
+ (NSMutableDictionary<NSString*, MTMathAtom*>*) supportedLatexSymbols
{
static NSMutableDictionary<NSString*, MTMathAtom*>* commands = nil;
if (!commands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
commands = [NSMutableDictionary dictionaryWithDictionary:@{
// Greek characters
@"alpha" : [MTMathAtom atomWithType:kMTMathAtomVariable value:@"\u03B1"],
Expand Down Expand Up @@ -883,15 +909,15 @@ + (nullable MTMathAtom *)tableWithEnvironment:(NSString *)env rows:(NSArray<NSAr
@"scriptstyle" : [[MTMathStyle alloc] initWithStyle:kMTLineStyleScript],
@"scriptscriptstyle" : [[MTMathStyle alloc] initWithStyle:kMTLineStyleScriptScript],
}];

}
});
return commands;
}

+ (NSDictionary*) aliases
{
static NSDictionary* aliases = nil;
if (!aliases) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
aliases = @{
@"lnot" : @"neg",
@"land" : @"wedge",
Expand Down Expand Up @@ -920,14 +946,15 @@ + (NSDictionary*) aliases
@"precnapprox" : @"nprecapprox",
@"succnapprox" : @"nsuccapprox",
};
}
});
return aliases;
}

+ (NSMutableDictionary<NSString*, NSMutableDictionary<NSNumber*, NSString*>*>*) textToLatexSymbolNames
{
static NSMutableDictionary<NSString*, NSMutableDictionary<NSNumber*, NSString*>*>* textToCommands = nil;
if (!textToCommands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
NSDictionary* commands = [self supportedLatexSymbols];
textToCommands = [NSMutableDictionary dictionaryWithCapacity:commands.count];
for (NSString* command in commands) {
Expand Down Expand Up @@ -957,14 +984,15 @@ + (NSDictionary*) aliases
}
inner[typeKey] = command;
}
}
});
return textToCommands;
}

+ (NSDictionary<NSString*, NSString*>*) accents
{
static NSDictionary* accents = nil;
if (!accents) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
accents = @{
@"grave" : @"\u0300",
@"acute" : @"\u0301",
Expand All @@ -979,14 +1007,15 @@ + (NSDictionary*) aliases
@"widehat" : @"\u0302",
@"widetilde" : @"\u0303",
};
}
});
return accents;
}

+ (NSDictionary*) accentValueToName
{
static NSDictionary* accentToCommands = nil;
if (!accentToCommands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
NSDictionary* accents = [self accents];
NSMutableDictionary* mutableDict = [NSMutableDictionary dictionaryWithCapacity:accents.count];
for (NSString* command in accents) {
Expand All @@ -1007,14 +1036,15 @@ + (NSDictionary*) accentValueToName
mutableDict[acc] = command;
}
accentToCommands = [mutableDict copy];
}
});
return accentToCommands;
}

+(NSDictionary<NSString*, NSString*> *) delimiters
{
static NSDictionary* delims = nil;
if (!delims) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
delims = @{
@"." : @"", // . means no delimiter
@"(" : @"(",
Expand Down Expand Up @@ -1049,14 +1079,15 @@ + (NSDictionary*) accentValueToName
@"lfloor" : @"\u230A",
@"rfloor" : @"\u230B",
};
}
});
return delims;
}

+ (NSDictionary*) delimValueToName
{
static NSDictionary* delimToCommands = nil;
if (!delimToCommands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
NSDictionary* delims = [self delimiters];
NSMutableDictionary* mutableDict = [NSMutableDictionary dictionaryWithCapacity:delims.count];
for (NSString* command in delims) {
Expand All @@ -1077,15 +1108,16 @@ + (NSDictionary*) delimValueToName
mutableDict[delim] = command;
}
delimToCommands = [mutableDict copy];
}
});
return delimToCommands;
}


+(NSDictionary<NSString*, NSNumber*> *) fontStyles
{
static NSDictionary<NSString*, NSNumber*>* fontStyles = nil;
if (!fontStyles) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
// \text* commands are handled by the parser via the textStyles
// dictionary, so they do NOT appear here.
fontStyles = @{
Expand All @@ -1106,7 +1138,7 @@ + (NSDictionary*) delimValueToName
@"mathbfit": @(kMTFontStyleBoldItalic),
@"bm": @(kMTFontStyleBoldItalic),
};
}
});
return fontStyles;
}

Expand All @@ -1115,7 +1147,8 @@ + (NSDictionary*) delimValueToName
+ (NSDictionary<NSString*, MTMathStackCommandSpec*>*) stackCommands
{
static NSDictionary* stackCommands = nil;
if (!stackCommands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
// Each command maps to a single stretchy cap glyph (a Unicode codepoint). The
// typesetter walks the cap's OpenType h_variants first; if no variant is wide
// enough, it falls back to the font's HorizontalGlyphAssembly (parts + connector
Expand All @@ -1142,7 +1175,7 @@ + (NSDictionary*) delimValueToName
@"stackrel": [[MTMathStackCommandSpec alloc] initWithOver:nil under:nil displayClass:kMTMathAtomRelation argRoles:@[@(kMTStackArgOver), @(kMTStackArgBase)] inheritsClass:NO],
@"stackbin": [[MTMathStackCommandSpec alloc] initWithOver:nil under:nil displayClass:kMTMathAtomBinaryOperator argRoles:@[@(kMTStackArgOver), @(kMTStackArgBase)] inheritsClass:NO],
};
}
});
return stackCommands;
}

Expand Down Expand Up @@ -1192,7 +1225,8 @@ + (MTMathAtomType) inheritedDisplayClassForBase:(MTMathList*)base
+ (NSDictionary<NSString*, NSString*>*) stackCommandReverseTable
{
static NSDictionary* reverseTable = nil;
if (!reverseTable) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
NSDictionary<NSString*, MTMathStackCommandSpec*>* forward = [self stackCommands];
NSMutableDictionary* mutable = [NSMutableDictionary dictionaryWithCapacity:forward.count];
for (NSString* cmd in forward) {
Expand All @@ -1201,7 +1235,7 @@ + (MTMathAtomType) inheritedDisplayClassForBase:(MTMathList*)base
mutable[key] = cmd;
}
reverseTable = [mutable copy];
}
});
return reverseTable;
}

Expand Down
20 changes: 12 additions & 8 deletions iosMath/lib/MTMathListBuilder.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,10 +607,11 @@ - (BOOL) expectCharacter:(unichar) ch
- (NSString*) readCommand
{
static NSSet<NSNumber*>* singleCharCommands = nil;
if (!singleCharCommands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
NSArray* singleChars = @[ @'{', @'}', @'$', @'#', @'%', @'_', @'|', @' ', @',', @'>', @';', @'!', @'\\' ];
singleCharCommands = [[NSSet alloc] initWithArray:singleChars];
}
});
if ([self hasCharacters]) {
// Check if we have a single character command.
unichar ch = [self getNextCharacter];
Expand Down Expand Up @@ -848,13 +849,14 @@ - (MTMathAtom*) atomForCommand:(NSString*) command
- (MTMathList*) stopCommand:(NSString*) command list:(MTMathList*) list stopChar:(unichar) stopChar
{
static NSDictionary<NSString*, NSArray*>* fractionCommands = nil;
if (!fractionCommands) {
static dispatch_once_t fractionCommandsOnce;
dispatch_once(&fractionCommandsOnce, ^{
fractionCommands = @{ @"over" : @[],
@"atop" : @[],
@"choose" : @[ @"(", @")"],
@"brack" : @[ @"[", @"]"],
@"brace" : @[ @"{", @"}"]};
}
});
if ([command isEqualToString:@"right"]) {
if (!_currentInnerAtom) {
NSString* errorMessage = @"Missing \\left";
Expand Down Expand Up @@ -1009,7 +1011,8 @@ - (MTMathAtom*) buildTable:(NSString*) env firstList:(MTMathList*) firstList row
+ (NSDictionary*) spaceToCommands
{
static NSDictionary* spaceToCommands = nil;
if (!spaceToCommands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
spaceToCommands = @{
@3 : @",",
@4 : @">",
Expand All @@ -1018,7 +1021,7 @@ + (NSDictionary*) spaceToCommands
@18 : @"quad",
@36 : @"qquad",
};
}
});
return spaceToCommands;
}

Expand Down Expand Up @@ -1093,14 +1096,15 @@ + (NSDictionary*) spaceToCommands
+ (NSDictionary*) styleToCommands
{
static NSDictionary* styleToCommands = nil;
if (!styleToCommands) {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
styleToCommands = @{
@(kMTLineStyleDisplay) : @"displaystyle",
@(kMTLineStyleText) : @"textstyle",
@(kMTLineStyleScript) : @"scriptstyle",
@(kMTLineStyleScriptScript) : @"scriptscriptstyle",
};
}
});
return styleToCommands;
}

Expand Down
Loading
Loading