[audit] Security, AX compliance, missing tests, error handling #5

New issue

Open

opened 2026-03-22 16:41:02 +00:00 by Virgil · 5 comments

Virgil commented

2026-03-22 16:41:02 +00:00

Member

Full audit:

Security: path traversal, injection, panics on untrusted input, race conditions
AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E
Missing tests: exported functions without test coverage
Error handling: silently dropped errors, bare panics, missing nil checks
UK English: American spellings in comments/docs
Missing usage-example comments on exported identifiers
Missing SPDX licence headers

Report all findings with severity and file:line. Do NOT fix.

Full audit: 1. Security: path traversal, injection, panics on untrusted input, race conditions 2. AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E 3. Missing tests: exported functions without test coverage 4. Error handling: silently dropped errors, bare panics, missing nil checks 5. UK English: American spellings in comments/docs 6. Missing usage-example comments on exported identifiers 7. Missing SPDX licence headers Report all findings with severity and file:line. Do NOT fix.

Virgil commented

2026-03-22 19:34:04 +00:00

Author

Member

Codex Audit Findings

HIGH (1)

Chat-template injection — raw Role/Content/prompt concatenated into control-token templates (generate.go:431/:445/:455, tokenizer.go:401)

MEDIUM (3)

mlxlm attention inspection trusts subprocess metadata, panics on partial data (backend.go:483/:497)
LoadSafetensors swallows load failures, no recoverable error path (io.go:22)
Shared model state unsynchronised, compiledGELU lazily initialised without locking (generate.go:57, batch.go)

## Codex Audit Findings ### HIGH (1) 1. Chat-template injection — raw Role/Content/prompt concatenated into control-token templates (generate.go:431/:445/:455, tokenizer.go:401) ### MEDIUM (3) 2. mlxlm attention inspection trusts subprocess metadata, panics on partial data (backend.go:483/:497) 3. LoadSafetensors swallows load failures, no recoverable error path (io.go:22) 4. Shared model state unsynchronised, compiledGELU lazily initialised without locking (generate.go:57, batch.go)

Virgil commented

2026-03-23 07:48:59 +00:00

Author

Member

Fix Applied

Commit 570b7fb: fix(api): address issue 5 findings

Chat-template injection sanitised in generate.go and tokenizer.go
mlxlm attention inspection no longer panics on partial data
LoadSafetensors errors properly surfaced
Shared model state synchronised, compiledGELU locked
554 additions across 13 files with new tests (generate_test.go 121 lines, backend_test.go 36 lines)

## Fix Applied Commit 570b7fb: fix(api): address issue 5 findings - Chat-template injection sanitised in generate.go and tokenizer.go - mlxlm attention inspection no longer panics on partial data - LoadSafetensors errors properly surfaced - Shared model state synchronised, compiledGELU locked - 554 additions across 13 files with new tests (generate_test.go 121 lines, backend_test.go 36 lines)

Virgil commented

2026-03-23 09:06:23 +00:00

Author

Member

Verification: FAIL

HIGH: CGo lifetime bug in LoadSafetensors — runtime.KeepAlive(holder) placed before iteration starts, not after. GC can free C maps during iteration. Needs KeepAlive after iteration completes.

## Verification: FAIL HIGH: CGo lifetime bug in LoadSafetensors — runtime.KeepAlive(holder) placed before iteration starts, not after. GC can free C maps during iteration. Needs KeepAlive after iteration completes.

Virgil commented

2026-03-23 09:12:56 +00:00

Author

Member

Fix Round 2

Commit 76441c4: fix(metal): keep safetensors iterator alive

runtime.KeepAlive moved after iteration completes
84-line GC pressure test added
Dispatching verification.

## Fix Round 2 Commit 76441c4: fix(metal): keep safetensors iterator alive - runtime.KeepAlive moved after iteration completes - 84-line GC pressure test added Dispatching verification.

Virgil commented

2026-03-23 09:19:35 +00:00

Author

Member

Verification: PASS

Round 2 — CGo lifetime fix verified. Chat-template escaping, safetensors iterator GC pressure, mlxlm inspect tests all pass. No blocking bugs or security regressions.

## Verification: PASS Round 2 — CGo lifetime fix verified. Chat-template escaping, safetensors iterator GC pressure, mlxlm inspect tests all pass. No blocking bugs or security regressions.