feat: Add memory limit enforcement for USDA/USDC parsers

[syoyo]

Implement configurable memory limits to prevent out-of-memory conditions when
parsing large USD files. Memory usage is tracked during parsing and loading
fails gracefully when limits are exceeded.

USDC Parser:

• Add memory tracking via CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros
• Track allocations in crate-reader.cc for tokens, arrays, and data structures
• Pass memory limit from USDLoadOptions through USDCReaderConfig
• Initialize _memoryUsage in CrateReader constructor

USDA Parser:

• Add max_memory_limit_in_mb to USDAReaderConfig (default 16GB)
• Add memory tracking members to AsciiParser class (default 128GB)
• Implement SetMaxMemoryLimit() method in AsciiParser
• Track allocations for arrays, references, and string data
• Add CHECK_MEMORY_USAGE calls in push_back operations

Both parsers now:

• Respect USDLoadOptions::max_memory_limit_in_mb setting
• Provide clear error messages showing limit and current usage
• Fail gracefully before system runs out of memory

Testing:

• Verified with suzanne-subd-lv5.usdc/usda at various limits (10MB, 50MB, 500MB)
• Small files load successfully with 1MB limit
• Memory limits properly enforced at different parsing stages

This feature is especially useful for:

• Mobile/embedded devices with limited memory
• Server environments with resource quotas
• Preventing DoS from malicious/corrupted USD files
• Debugging memory usage patterns in USD files

[claude]

Claude finished @syoyo's task —— View job

---

Code Review: Memory Limit Enforcement for USDA/USDC Parsers

I've completed a comprehensive review of the memory limit enforcement implementation. Here's my detailed analysis:

Todo List:

• Read and analyze all changed files
• Review memory tracking macros and implementation
• Check error handling and messaging consistency
• Evaluate security implications
• Assess performance considerations
• Review test coverage needs
• Provide comprehensive feedback and recommendations

---

Overall Assessment ✅

This is a well-implemented security feature that addresses a critical need for preventing out-of-memory conditions when parsing potentially malicious or corrupted USD files. The implementation follows good defensive programming practices.

Code Quality & Best Practices

✅ Strengths:

1. Consistent Implementation Pattern: The same memory tracking approach is used across both USDA and USDC parsers with similar CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros.

2. Proper Integration: Memory limits are properly passed through the configuration chain:

   • USDLoadOptions::max_memory_limit_in_mb → USDAReaderConfig::max_memory_limit_in_mb → AsciiParser::SetMaxMemoryLimit()
   • USDLoadOptions → USDCReaderConfig::kMaxAllowedMemoryInMB → CrateReader constructor

3. Good Error Messages: Both parsers provide clear error messages showing current usage vs. limit in MB.

4. Reasonable Defaults: 16GB for USDA, 128GB for AsciiParser internal limit - appropriate for modern systems.

⚠️ Areas for Improvement:

1. Inconsistent Variable Naming:

   • USDA parser uses _memory_usage (snake_case)
   • USDC parser uses _memoryUsage (camelCase)
   • Recommendation: Standardize on one naming convention across the codebase

2. REDUCE_MEMORY_USAGE Bug in USDC Parser (crate-reader.cc:74-77):

   #define REDUCE_MEMORY_USAGE(__nbytes) do { \
     if (_memoryUsage < (__nbytes)) { \  // ❌ BUG: Should be >= not <
       _memoryUsage -= (__nbytes); \
     } \
   } while(0)

   This will cause integer underflow when _memoryUsage < __nbytes. Should be >= like in the USDA version.

Potential Bugs & Issues

🐛 Critical Bug:

The USDC parser's REDUCE_MEMORY_USAGE macro has a logic error that could cause memory usage counter underflow, leading to incorrect tracking and potential bypasses of memory limits.

⚠️ Memory Tracking Granularity:

The implementation tracks allocation sizes but doesn't account for:

• Memory fragmentation overhead
• Container overhead (std::vector capacity vs size)
• Temporary buffers during parsing

This could lead to actual memory usage being higher than tracked usage.

Security Considerations

✅ Security Strengths:

1. DoS Prevention: Effectively prevents memory exhaustion attacks from malicious USD files
2. Early Termination: Fails fast before system resources are exhausted
3. Consistent Enforcement: Applied across multiple data structures and allocation points
4. Clear Error Reporting: Helps with debugging and monitoring

🛡️ Security Recommendations:

1. Lower Default Limits: Consider reducing default limits for security-critical environments
2. Rate Limiting: Consider adding rate limiting for repeated parse attempts
3. Memory Pressure Detection: Could benefit from system memory pressure monitoring

Performance Considerations

✅ Performance Impact:

• Low Overhead: Simple integer arithmetic per allocation
• Macro-based: Minimal runtime cost when memory tracking is disabled
• Early Exit: Prevents wasted work on oversized files

🔄 Optimization Opportunities:

1. Batch Updates: For small allocations, could batch memory usage updates
2. Conditional Compilation: Consider #ifdef TINYUSDZ_ENABLE_MEMORY_TRACKING for production builds

Test Coverage Assessment

❌ Missing Test Coverage:

The PR description mentions manual testing but I don't see automated tests for:

1. Memory limit boundary conditions
2. Memory tracking accuracy
3. Error message formatting
4. Edge cases (very small limits, exact limit boundaries)
5. Recovery scenarios (freeing memory after failed parse)

📋 Recommended Tests:

// Example test cases needed:
- Parse with limit = file_size - 1 (should fail)
- Parse with limit = file_size + 1 (should succeed)  
- Verify error message format contains both limit and current usage
- Test REDUCE_MEMORY_USAGE functionality
- Test multiple parsers with different limits

Implementation Details Review

USDA Parser (ascii-parser-basetype.cc, ascii-parser.cc):

• ✅ Proper macro definitions with correct logic
• ✅ Comprehensive coverage of data structure allocations
• ✅ Consistent error message formatting
• ✅ Integration with AsciiParser class members

USDC Parser (crate-reader.cc):

• ✅ Extensive coverage of binary format parsing
• ✅ Memory tracking during decompression operations
• ❌ Bug in REDUCE_MEMORY_USAGE macro logic
• ✅ Integration with CrateReader constructor initialization

Configuration (tinyusdz.cc, usda-reader.hh):

• ✅ Proper parameter passing through configuration structures
• ✅ Sensible default values
• ✅ 32-bit vs 64-bit environment handling

Recommendations

High Priority:

1. Fix REDUCE_MEMORY_USAGE bug in crate-reader.cc:74 (change < to >=)
2. Add automated tests for memory limit enforcement
3. Standardize variable naming convention

Medium Priority:

1. Consider adding memory pressure monitoring
2. Add conditional compilation flags for memory tracking
3. Improve memory tracking granularity for containers

Low Priority:

1. Consider batch updates for small allocations
2. Add memory usage reporting/monitoring APIs
3. Consider adaptive limits based on available system memory

---

Summary

This is a solid security feature implementation that significantly improves TinyUSDZ's resilience against memory exhaustion attacks. The main issues are the critical bug in the USDC parser's memory reduction logic and lack of automated test coverage. Once the bug is fixed and tests are added, this will be a valuable addition to the library's security posture.

Recommendation: Approve after fixing the REDUCE_MEMORY_USAGE bug and adding basic automated tests.

---

[Copilot]

Pull Request Overview

This PR implements configurable memory limits for both USDA and USDC parsers to prevent out-of-memory conditions when parsing large USD files. The implementation adds memory tracking throughout the parsing process and fails gracefully when limits are exceeded.

Key changes:

• Added memory tracking macros and infrastructure to both USDA and USDC parsers
• Implemented configurable memory limits through USDLoadOptions with reasonable defaults (16GB for USDA, passed through for USDC)
• Added memory usage checks at key allocation points during parsing operations

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/usda-reader.hh	Adds max_memory_limit_in_mb configuration field with 16GB default
src/usda-reader.cc	Connects reader config to parser memory limit setting
src/tinyusdz.cc	Passes memory limits from USDLoadOptions to both USDA and USDC configs
src/crate-reader.cc	Initializes memory tracking in USDC reader and adds token allocation check
src/ascii-parser.hh	Adds memory tracking members and SetMaxMemoryLimit method with 128GB default
src/ascii-parser.cc	Implements CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros and adds memory checks
src/ascii-parser-basetype.cc	Duplicates memory tracking macros and adds checks for array/reference allocations

[Copilot]

The default memory limit of 128GB is inconsistent with the 16GB default in USDAReaderConfig. Consider using a consistent default value or making this configurable through the constructor to avoid confusion.

Suggested change

	size_t _max_memory_limit_bytes{128ull * 1024ull * 1024ull * 1024ull}; // Default 128GB
	size_t _max_memory_limit_bytes{16ull * 1024ull * 1024ull * 1024ull}; // Default 16GB (consistent with USDAReaderConfig)

[Copilot]

The CHECK_MEMORY_USAGE and REDUCE_MEMORY_USAGE macros are duplicated in both ascii-parser.cc and ascii-parser-basetype.cc. Consider moving these to a shared header file to avoid code duplication and ensure consistency.

[Copilot]

The CHECK_MEMORY_USAGE and REDUCE_MEMORY_USAGE macros are duplicated from ascii-parser.cc. Consider moving these to a shared header file to avoid code duplication and ensure consistency.

[Copilot]

The REDUCE_MEMORY_USAGE macro is defined but never used in the codebase. This suggests either incomplete implementation or dead code that should be removed.

Suggested change

	#define REDUCE_MEMORY_USAGE(__nbytes) do { \
	if (_memory_usage >= (__nbytes)) { \
	_memory_usage -= (__nbytes); \
	} \
	} while(0)

[Copilot]

The REDUCE_MEMORY_USAGE macro is defined but never used in the codebase. This suggests either incomplete implementation or dead code that should be removed.

Suggested change

	#define REDUCE_MEMORY_USAGE(__nbytes) do { \
	if (_memory_usage >= (__nbytes)) { \
	_memory_usage -= (__nbytes); \
	} \
	} while(0)

[claude]

Claude finished @syoyo's task —— View job

---

Code Review: Memory Limit Enforcement for USDA/USDC Parsers

Todo List:

• Read and analyze key changed files for memory limit implementation
• Review memory tracking macros and their implementation
• Check consistency between USDA and USDC parser implementations
• Look for potential bugs in memory tracking logic
• Evaluate security implications and DoS prevention
• Assess performance considerations
• Review test coverage and recommendations
• Provide comprehensive feedback and recommendations

---

Overall Assessment ✅

This is a well-implemented security feature that addresses a critical need for preventing out-of-memory conditions when parsing potentially malicious or corrupted USD files. The implementation follows good defensive programming practices with consistent patterns across both parsers.

Key Findings

🐛 Critical Bug Found

Location: src/crate-reader.cc:74

#define REDUCE_MEMORY_USAGE(__nbytes) do { \
  if (_memoryUsage < (__nbytes)) { \    // ❌ BUG: Should be >= not <
    _memoryUsage -= (__nbytes); \
  } \
} while(0)

Issue: This condition is inverted! It should check _memoryUsage >= (__nbytes) before subtracting, not <. The current logic will cause integer underflow when _memoryUsage < __nbytes, leading to incorrect memory tracking and potential security bypasses.

Fix: Change line 74 from if (_memoryUsage < (__nbytes)) to if (_memoryUsage >= (__nbytes))

⚠️ Inconsistencies Found

1. Variable Naming Convention:

   • USDA parser: _memory_usage (snake_case) in ascii-parser.hh:880
   • USDC parser: _memoryUsage (camelCase) in crate-reader.cc:86
   • Recommendation: Standardize on one naming convention

2. Default Memory Limits:

   • USDAReaderConfig: 16GB (usda-reader.hh:21)
   • AsciiParser: 128GB (ascii-parser.hh:879)
   • Recommendation: Use consistent default values to avoid confusion

3. Macro Definitions:

   • Same macros duplicated in both ascii-parser.cc and ascii-parser-basetype.cc
   • Recommendation: Move to shared header to avoid duplication

Code Quality Assessment

✅ Strengths:

1. Consistent Implementation Pattern: Both parsers use similar CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros
2. Proper Configuration Flow: Memory limits correctly passed through:
   • USDLoadOptions::max_memory_limit_in_mb → USDAReaderConfig::max_memory_limit_in_mb → AsciiParser::SetMaxMemoryLimit()
   • USDLoadOptions → USDCReaderConfig::kMaxAllowedMemoryInMB → CrateReader constructor
3. Clear Error Messages: Both parsers provide helpful error messages showing current vs limit
4. Comprehensive Coverage: Memory tracking applied to arrays, strings, references, and complex data structures

🔧 Areas for Improvement:

1. Memory Tracking Granularity: Current implementation tracks allocation sizes but doesn't account for:

   • Memory fragmentation overhead
   • Container overhead (std::vector capacity vs size)
   • Temporary buffers during parsing

2. Unused Code: REDUCE_MEMORY_USAGE macro is defined in ascii-parser.cc but commented out and never used (lines 70-76)

Security Analysis

🛡️ Security Strengths:

• DoS Prevention: Effectively prevents memory exhaustion attacks
• Early Termination: Fails fast before system resources are exhausted
• Consistent Enforcement: Applied across multiple allocation points
• Clear Error Reporting: Helps with monitoring and debugging

🚨 Security Concerns:

1. Critical Bug Impact: The USDC parser bug could allow attackers to bypass memory limits by causing integer underflow
2. Memory Tracking Accuracy: Actual memory usage may exceed tracked usage due to overhead not being accounted for

Performance Considerations

✅ Low Performance Impact:

• Simple integer arithmetic per allocation
• Macro-based implementation with minimal runtime cost
• Early exit prevents wasted work on oversized files

🔄 Optimization Opportunities:

• Consider conditional compilation for production builds
• Batch updates for small allocations to reduce tracking overhead

Test Coverage Assessment

❌ Missing Automated Tests:

The PR mentions manual testing but lacks automated tests for:

• Memory limit boundary conditions
• Memory tracking accuracy verification
• Error message format validation
• Edge cases (very small limits, exact boundaries)
• Recovery scenarios after failed parsing

📋 Recommended Test Cases:

// Critical test cases needed:
1. Parse file with limit = actual_usage - 1MB (should fail)
2. Parse file with limit = actual_usage + 1MB (should succeed)
3. Verify REDUCE_MEMORY_USAGE functionality works correctly
4. Test multiple parser instances with different limits
5. Validate error message contains both limit and current usage
6. Test integer overflow/underflow edge cases

Recommendations

High Priority (Must Fix):

1. Fix the REDUCE_MEMORY_USAGE bug in crate-reader.cc:74
2. Add automated test suite for memory limit enforcement
3. Remove unused/commented code in ascii-parser.cc:70-76

Medium Priority:

1. Standardize naming conventions across parsers
2. Consolidate macro definitions in shared header
3. Align default memory limits between configs
4. Improve memory tracking accuracy for container overhead

Low Priority:

1. Add conditional compilation flags for memory tracking
2. Consider system memory pressure monitoring
3. Add memory usage reporting APIs

Summary

This is a solid security feature that significantly improves TinyUSDZ's resilience against memory-based attacks. The main issues are the critical bug in USDC parser memory reduction logic and lack of automated test coverage.

Recommendation: Requires fixes before merge - specifically the REDUCE_MEMORY_USAGE bug which could compromise the security benefits of this feature.

---

[claude]

Claude finished @syoyo's task —— View job

---

Code Review: Memory Limit Enforcement for USDA/USDC Parsers

Todo List:

• Read and analyze key changed files for memory limit implementation
• Review memory tracking macros and their implementation
• Check for the previously identified REDUCE_MEMORY_USAGE bug
• Evaluate consistency between USDA and USDC parser implementations
• Assess security implications and DoS prevention capabilities
• Review performance considerations and overhead
• Evaluate test coverage and recommendations
• Provide comprehensive feedback and recommendations

---

Overall Assessment ✅

This is a well-implemented security feature that addresses a critical need for preventing out-of-memory conditions when parsing potentially malicious or corrupted USD files. The implementation follows good defensive programming practices with consistent patterns across both parsers.

🐛 Critical Bug Found

Location: src/crate-reader.cc:74

#define REDUCE_MEMORY_USAGE(__nbytes) do { \
  if (_memoryUsage < (__nbytes)) { \    // ❌ BUG: Should be >= not <
    _memoryUsage -= (__nbytes); \
  } \
} while(0)

Issue: This condition is inverted! It should check _memoryUsage >= (__nbytes) before subtracting, not <. The current logic will cause integer underflow when _memoryUsage < __nbytes, leading to incorrect memory tracking and potential security bypasses.

Fix: Change line 74 from if (_memoryUsage < (__nbytes)) to if (_memoryUsage >= (__nbytes))

Key Findings

⚠️ Inconsistencies Found

1. Variable Naming Convention:

   • USDA parser: _memory_usage (snake_case) in ascii-parser.hh:880
   • USDC parser: _memoryUsage (camelCase) in crate-reader.cc:86
   • Recommendation: Standardize on one naming convention

2. Default Memory Limits:

   • USDAReaderConfig: 16GB (usda-reader.hh:21)
   • AsciiParser: 128GB (ascii-parser.hh:879)
   • USDCReaderConfig: 16GB (usdc-reader.hh:24)
   • Recommendation: Use consistent default values to avoid confusion

3. Macro Definitions:

   • Same macros duplicated in both ascii-parser.cc and ascii-parser-basetype.cc
   • Recommendation: Move to shared header to avoid duplication

4. Unused Code:

   • REDUCE_MEMORY_USAGE macro is defined in ascii-parser.cc but commented out and never used (lines 70-76)

Code Quality Assessment

✅ Strengths:

1. Consistent Implementation Pattern: Both parsers use similar CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros
2. Proper Configuration Flow: Memory limits correctly passed through:
   • USDLoadOptions::max_memory_limit_in_mb → USDAReaderConfig::max_memory_limit_in_mb → AsciiParser::SetMaxMemoryLimit()
   • USDLoadOptions::max_memory_limit_in_mb → USDCReaderConfig::kMaxAllowedMemoryInMB → CrateReader constructor
3. Clear Error Messages: Both parsers provide helpful error messages showing current vs limit
4. Comprehensive Coverage: Memory tracking applied to arrays, strings, references, and complex data structures

🔧 Areas for Improvement:

1. Memory Tracking Granularity: Current implementation tracks allocation sizes but doesn't account for:
   • Memory fragmentation overhead
   • Container overhead (std::vector capacity vs size)
   • Temporary buffers during parsing

Security Analysis

🛡️ Security Strengths:

• DoS Prevention: Effectively prevents memory exhaustion attacks
• Early Termination: Fails fast before system resources are exhausted
• Consistent Enforcement: Applied across multiple allocation points
• Clear Error Reporting: Helps with monitoring and debugging

🚨 Security Concerns:

1. Critical Bug Impact: The USDC parser bug could allow attackers to bypass memory limits by causing integer underflow
2. Memory Tracking Accuracy: Actual memory usage may exceed tracked usage due to overhead not being accounted for

Performance Considerations

✅ Low Performance Impact:

• Simple integer arithmetic per allocation
• Macro-based implementation with minimal runtime cost
• Early exit prevents wasted work on oversized files

🔄 Optimization Opportunities:

• Consider conditional compilation for production builds
• Batch updates for small allocations to reduce tracking overhead

Test Coverage Assessment

❌ Missing Automated Tests:

The PR mentions manual testing but lacks automated tests for:

• Memory limit boundary conditions
• Memory tracking accuracy verification
• Error message format validation
• Edge cases (very small limits, exact boundaries)
• Recovery scenarios after failed parsing

📋 Recommended Test Cases:

// Critical test cases needed:
1. Parse file with limit = actual_usage - 1MB (should fail)
2. Parse file with limit = actual_usage + 1MB (should succeed)
3. Verify REDUCE_MEMORY_USAGE functionality works correctly
4. Test multiple parser instances with different limits
5. Validate error message contains both limit and current usage
6. Test integer overflow/underflow edge cases

Implementation Details Review

USDA Parser (ascii-parser-basetype.cc, ascii-parser.cc):

• ✅ Proper macro definitions with correct logic
• ✅ Comprehensive coverage of data structure allocations
• ✅ Consistent error message formatting
• ✅ Integration with AsciiParser class members

USDC Parser (crate-reader.cc):

• ✅ Extensive coverage of binary format parsing
• ✅ Memory tracking during decompression operations
• ❌ Bug in REDUCE_MEMORY_USAGE macro logic
• ✅ Integration with CrateReader constructor initialization

Configuration (tinyusdz.cc, usda-reader.hh):

• ✅ Proper parameter passing through configuration structures
• ✅ Sensible default values
• ✅ 32-bit vs 64-bit environment handling

Recommendations

High Priority (Must Fix):

1. Fix the REDUCE_MEMORY_USAGE bug in crate-reader.cc:74
2. Add automated test suite for memory limit enforcement
3. Remove unused/commented code in ascii-parser.cc:70-76

Medium Priority:

1. Standardize naming conventions across parsers
2. Consolidate macro definitions in shared header
3. Align default memory limits between configs
4. Improve memory tracking accuracy for container overhead

Low Priority:

1. Add conditional compilation flags for memory tracking
2. Consider system memory pressure monitoring
3. Add memory usage reporting APIs

Summary

This is a solid security feature that significantly improves TinyUSDZ's resilience against memory-based attacks. The main issues are the critical bug in USDC parser memory reduction logic and lack of automated test coverage.

Recommendation: Requires fixes before merge - specifically the REDUCE_MEMORY_USAGE bug which could compromise the security benefits of this feature.

---

[claude]

Claude finished @syoyo's task —— View job

---

Code Review: Memory Limit Enforcement for USDA/USDC Parsers

Todo List:

• Read and analyze key changed files for memory limit implementation
• Review memory tracking macros and their implementation
• Check for the previously identified REDUCE_MEMORY_USAGE bug
• Evaluate consistency between USDA and USDC parser implementations
• Assess security implications and DoS prevention capabilities
• Review performance considerations and overhead
• Evaluate test coverage and recommendations
• Provide comprehensive feedback and recommendations

---

Overall Assessment ✅

This is a well-implemented security feature that addresses a critical need for preventing out-of-memory conditions when parsing potentially malicious or corrupted USD files. The implementation follows good defensive programming practices with consistent patterns across both parsers.

🐛 Critical Bug Found

Location: src/crate-reader.cc:74

#define REDUCE_MEMORY_USAGE(__nbytes) do { \
  if (_memoryUsage < (__nbytes)) { \    // ❌ BUG: Should be >= not <
    _memoryUsage -= (__nbytes); \
  } \
} while(0)

Issue: This condition is inverted! It should check _memoryUsage >= (__nbytes) before subtracting, not <. The current logic will cause integer underflow when _memoryUsage < __nbytes, leading to incorrect memory tracking and potential security bypasses.

Fix: Change line 74 from if (_memoryUsage < (__nbytes)) to if (_memoryUsage >= (__nbytes))

Key Findings

⚠️ Inconsistencies Found

1. Variable Naming Convention:

   • USDA parser: _memory_usage (snake_case) in ascii-parser.hh:880
   • USDC parser: _memoryUsage (camelCase) in crate-reader.cc:86
   • Recommendation: Standardize on one naming convention

2. Default Memory Limits:

   • USDAReaderConfig: 16GB (usda-reader.hh:21)
   • AsciiParser: 128GB (ascii-parser.hh:879)
   • USDCReaderConfig: 16GB (usdc-reader.hh:24)
   • Recommendation: Use consistent default values to avoid confusion

3. Macro Definitions:

   • Same macros duplicated in both ascii-parser.cc and ascii-parser-basetype.cc
   • Recommendation: Move to shared header to avoid duplication

4. Unused Code:

   • REDUCE_MEMORY_USAGE macro is defined in ascii-parser.cc but commented out and never used (lines 70-76)

Code Quality Assessment

✅ Strengths:

1. Consistent Implementation Pattern: Both parsers use similar CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros
2. Proper Configuration Flow: Memory limits correctly passed through:
   • USDLoadOptions::max_memory_limit_in_mb → USDAReaderConfig::max_memory_limit_in_mb → AsciiParser::SetMaxMemoryLimit()
   • USDLoadOptions::max_memory_limit_in_mb → USDCReaderConfig::kMaxAllowedMemoryInMB → CrateReader constructor
3. Clear Error Messages: Both parsers provide helpful error messages showing current vs limit
4. Comprehensive Coverage: Memory tracking applied to arrays, strings, references, and complex data structures

🔧 Areas for Improvement:

1. Memory Tracking Granularity: Current implementation tracks allocation sizes but doesn't account for:
   • Memory fragmentation overhead
   • Container overhead (std::vector capacity vs size)
   • Temporary buffers during parsing

Security Analysis

🛡️ Security Strengths:

• DoS Prevention: Effectively prevents memory exhaustion attacks
• Early Termination: Fails fast before system resources are exhausted
• Consistent Enforcement: Applied across multiple allocation points
• Clear Error Reporting: Helps with monitoring and debugging

🚨 Security Concerns:

1. Critical Bug Impact: The USDC parser bug could allow attackers to bypass memory limits by causing integer underflow
2. Memory Tracking Accuracy: Actual memory usage may exceed tracked usage due to overhead not being accounted for

Performance Considerations

✅ Low Performance Impact:

• Simple integer arithmetic per allocation
• Macro-based implementation with minimal runtime cost
• Early exit prevents wasted work on oversized files

🔄 Optimization Opportunities:

• Consider conditional compilation for production builds
• Batch updates for small allocations to reduce tracking overhead

Test Coverage Assessment

❌ Missing Automated Tests:

The PR mentions manual testing but lacks automated tests for:

• Memory limit boundary conditions
• Memory tracking accuracy verification
• Error message format validation
• Edge cases (very small limits, exact boundaries)
• Recovery scenarios after failed parsing

📋 Recommended Test Cases:

// Critical test cases needed:
1. Parse file with limit = actual_usage - 1MB (should fail)
2. Parse file with limit = actual_usage + 1MB (should succeed)
3. Verify REDUCE_MEMORY_USAGE functionality works correctly
4. Test multiple parser instances with different limits
5. Validate error message contains both limit and current usage
6. Test integer overflow/underflow edge cases

Implementation Details Review

USDA Parser (ascii-parser-basetype.cc, ascii-parser.cc):

• ✅ Proper macro definitions with correct logic
• ✅ Comprehensive coverage of data structure allocations
• ✅ Consistent error message formatting
• ✅ Integration with AsciiParser class members

USDC Parser (crate-reader.cc):

• ✅ Extensive coverage of binary format parsing
• ✅ Memory tracking during decompression operations
• ❌ Bug in REDUCE_MEMORY_USAGE macro logic
• ✅ Integration with CrateReader constructor initialization

Configuration (tinyusdz.cc, usda-reader.hh):

• ✅ Proper parameter passing through configuration structures
• ✅ Sensible default values
• ✅ 32-bit vs 64-bit environment handling

Recommendations

High Priority (Must Fix):

1. Fix the REDUCE_MEMORY_USAGE bug in crate-reader.cc:74
2. Add automated test suite for memory limit enforcement
3. Remove unused/commented code in ascii-parser.cc:70-76

Medium Priority:

1. Standardize naming conventions across parsers
2. Consolidate macro definitions in shared header
3. Align default memory limits between configs
4. Improve memory tracking accuracy for container overhead

Low Priority:

1. Add conditional compilation flags for memory tracking
2. Consider system memory pressure monitoring
3. Add memory usage reporting APIs

Summary

This is a solid security feature that significantly improves TinyUSDZ's resilience against memory-based attacks. The main issues are the critical bug in USDC parser memory reduction logic and lack of automated test coverage.

Recommendation: Requires fixes before merge - specifically the REDUCE_MEMORY_USAGE bug which could compromise the security benefits of this feature.

---

[claude]

Claude finished @syoyo's task —— View job

---

Code Review: Memory Limit Enforcement for USDA/USDC Parsers

Todo List:

• Read and analyze key changed files for memory limit implementation
• Review memory tracking macros and their implementation
• Check for potential bugs in memory tracking logic
• Evaluate consistency between USDA and USDC parser implementations
• Assess security implications and DoS prevention capabilities
• Review performance considerations and overhead
• Evaluate test coverage and recommendations
• Provide comprehensive feedback and recommendations

---

Overall Assessment ✅

This is a well-implemented security feature that addresses a critical need for preventing out-of-memory conditions when parsing potentially malicious or corrupted USD files. The implementation follows good defensive programming practices with consistent patterns across both parsers.

🐛 Critical Bug Found

Location: src/crate-reader.cc:74

#define REDUCE_MEMORY_USAGE(__nbytes) do { \
  if (_memoryUsage < (__nbytes)) { \    // ❌ BUG: Should be >= not <
    _memoryUsage -= (__nbytes); \
  } \
} while(0)

Issue: This condition is inverted! It should check _memoryUsage >= (__nbytes) before subtracting, not <. The current logic will cause integer underflow when _memoryUsage < __nbytes, leading to incorrect memory tracking and potential security bypasses.

Fix: Change line 74 from if (_memoryUsage < (__nbytes)) to if (_memoryUsage >= (__nbytes))

Key Findings

⚠️ Inconsistencies Found

1. Variable Naming Convention:

   • USDA parser: _memory_usage (snake_case) in ascii-parser.hh:880
   • USDC parser: _memoryUsage (camelCase) in crate-reader.cc:86
   • Recommendation: Standardize on one naming convention

2. Default Memory Limits:

   • USDAReaderConfig: 16GB (usda-reader.hh:21)
   • AsciiParser: 128GB (ascii-parser.hh:879)
   • USDCReaderConfig: 16GB (usdc-reader.hh:24)
   • Recommendation: Use consistent default values to avoid confusion

3. Macro Definitions:

   • Same macros duplicated in both ascii-parser.cc and ascii-parser-basetype.cc
   • Recommendation: Move to shared header to avoid duplication

4. Unused Code:

   • REDUCE_MEMORY_USAGE macro is defined in ascii-parser.cc but commented out and never used (lines 70-76)

Code Quality Assessment

✅ Strengths:

1. Consistent Implementation Pattern: Both parsers use similar CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros
2. Proper Configuration Flow: Memory limits correctly passed through:
   • USDLoadOptions::max_memory_limit_in_mb → USDAReaderConfig::max_memory_limit_in_mb → AsciiParser::SetMaxMemoryLimit()
   • USDLoadOptions::max_memory_limit_in_mb → USDCReaderConfig::kMaxAllowedMemoryInMB → CrateReader constructor
3. Clear Error Messages: Both parsers provide helpful error messages showing current vs limit
4. Comprehensive Coverage: Memory tracking applied to arrays, strings, references, and complex data structures

🔧 Areas for Improvement:

1. Memory Tracking Granularity: Current implementation tracks allocation sizes but doesn't account for:
   • Memory fragmentation overhead
   • Container overhead (std::vector capacity vs size)
   • Temporary buffers during parsing

Security Analysis

🛡️ Security Strengths:

• DoS Prevention: Effectively prevents memory exhaustion attacks
• Early Termination: Fails fast before system resources are exhausted
• Consistent Enforcement: Applied across multiple allocation points
• Clear Error Reporting: Helps with monitoring and debugging

🚨 Security Concerns:

1. Critical Bug Impact: The USDC parser bug could allow attackers to bypass memory limits by causing integer underflow
2. Memory Tracking Accuracy: Actual memory usage may exceed tracked usage due to overhead not being accounted for

Performance Considerations

✅ Low Performance Impact:

• Simple integer arithmetic per allocation
• Macro-based implementation with minimal runtime cost
• Early exit prevents wasted work on oversized files

🔄 Optimization Opportunities:

• Consider conditional compilation for production builds
• Batch updates for small allocations to reduce tracking overhead

Test Coverage Assessment

❌ Missing Automated Tests:

The PR mentions manual testing but lacks automated tests for:

• Memory limit boundary conditions
• Memory tracking accuracy verification
• Error message format validation
• Edge cases (very small limits, exact boundaries)
• Recovery scenarios after failed parsing

📋 Recommended Test Cases:

// Critical test cases needed:
1. Parse file with limit = actual_usage - 1MB (should fail)
2. Parse file with limit = actual_usage + 1MB (should succeed)
3. Verify REDUCE_MEMORY_USAGE functionality works correctly
4. Test multiple parser instances with different limits
5. Validate error message contains both limit and current usage
6. Test integer overflow/underflow edge cases

Implementation Details Review

USDA Parser (ascii-parser-basetype.cc, ascii-parser.cc):

• ✅ Proper macro definitions with correct logic
• ✅ Comprehensive coverage of data structure allocations
• ✅ Consistent error message formatting
• ✅ Integration with AsciiParser class members

USDC Parser (crate-reader.cc):

• ✅ Extensive coverage of binary format parsing
• ✅ Memory tracking during decompression operations
• ❌ Bug in REDUCE_MEMORY_USAGE macro logic
• ✅ Integration with CrateReader constructor initialization

Configuration (tinyusdz.cc, usda-reader.hh):

• ✅ Proper parameter passing through configuration structures
• ✅ Sensible default values
• ✅ 32-bit vs 64-bit environment handling

Recommendations

High Priority (Must Fix):

1. Fix the REDUCE_MEMORY_USAGE bug in crate-reader.cc:74
2. Add automated test suite for memory limit enforcement
3. Remove unused/commented code in ascii-parser.cc:70-76

Medium Priority:

1. Standardize naming conventions across parsers
2. Consolidate macro definitions in shared header
3. Align default memory limits between configs
4. Improve memory tracking accuracy for container overhead

Low Priority:

1. Add conditional compilation flags for memory tracking
2. Consider system memory pressure monitoring
3. Add memory usage reporting APIs

Summary

This is a solid security feature that significantly improves TinyUSDZ's resilience against memory-based attacks. The main issues are the critical bug in USDC parser memory reduction logic and lack of automated test coverage.

Recommendation: Requires fixes before merge - specifically the REDUCE_MEMORY_USAGE bug which could compromise the security benefits of this feature.

---

[claude]

Claude finished @syoyo's task —— View job

---

Code Review: Memory Limit Enforcement for USDA/USDC Parsers

Todo List:

• Read and analyze key changed files for memory limit implementation
• Review memory tracking macros and their implementation
• Check for potential bugs in memory tracking logic
• Evaluate consistency between USDA and USDC parser implementations
• Assess security implications and DoS prevention capabilities
• Review performance considerations and overhead
• Evaluate test coverage and recommendations
• Provide comprehensive feedback and recommendations

---

Overall Assessment ✅

This is a well-implemented security feature that addresses a critical need for preventing out-of-memory conditions when parsing potentially malicious or corrupted USD files. The implementation follows good defensive programming practices with consistent patterns across both parsers.

🐛 Critical Bug Found

Location: src/crate-reader.cc:74

#define REDUCE_MEMORY_USAGE(__nbytes) do { \
  if (_memoryUsage < (__nbytes)) { \    // ❌ BUG: Should be >= not <
    _memoryUsage -= (__nbytes); \
  } \
} while(0)

Issue: This condition is inverted! It should check _memoryUsage >= (__nbytes) before subtracting, not <. The current logic will cause integer underflow when _memoryUsage < __nbytes, leading to incorrect memory tracking and potential security bypasses.

Fix: Change line 74 from if (_memoryUsage < (__nbytes)) to if (_memoryUsage >= (__nbytes))

Key Findings

⚠️ Inconsistencies Found

1. Variable Naming Convention:

   • USDA parser: _memory_usage (snake_case) in ascii-parser.hh:880
   • USDC parser: _memoryUsage (camelCase) in crate-reader.cc:86
   • Recommendation: Standardize on one naming convention

2. Default Memory Limits:

   • USDAReaderConfig: 16GB (usda-reader.hh:21)
   • AsciiParser: 128GB (ascii-parser.hh:879)
   • USDCReaderConfig: 16GB (usdc-reader.hh:24)
   • Recommendation: Use consistent default values to avoid confusion

3. Macro Definitions:

   • Same macros duplicated in both ascii-parser.cc and ascii-parser-basetype.cc
   • Recommendation: Move to shared header to avoid duplication

4. Unused Code:

   • REDUCE_MEMORY_USAGE macro is defined in ascii-parser.cc but commented out and never used (lines 70-76)

Code Quality Assessment

✅ Strengths:

1. Consistent Implementation Pattern: Both parsers use similar CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros
2. Proper Configuration Flow: Memory limits correctly passed through:
   • USDLoadOptions::max_memory_limit_in_mb → USDAReaderConfig::max_memory_limit_in_mb → AsciiParser::SetMaxMemoryLimit()
   • USDLoadOptions::max_memory_limit_in_mb → USDCReaderConfig::kMaxAllowedMemoryInMB → CrateReader constructor
3. Clear Error Messages: Both parsers provide helpful error messages showing current vs limit
4. Comprehensive Coverage: Memory tracking applied to arrays, strings, references, and complex data structures

🔧 Areas for Improvement:

1. Memory Tracking Granularity: Current implementation tracks allocation sizes but doesn't account for:
   • Memory fragmentation overhead
   • Container overhead (std::vector capacity vs size)
   • Temporary buffers during parsing

Security Analysis

🛡️ Security Strengths:

• DoS Prevention: Effectively prevents memory exhaustion attacks
• Early Termination: Fails fast before system resources are exhausted
• Consistent Enforcement: Applied across multiple allocation points
• Clear Error Reporting: Helps with monitoring and debugging

🚨 Security Concerns:

1. Critical Bug Impact: The USDC parser bug could allow attackers to bypass memory limits by causing integer underflow
2. Memory Tracking Accuracy: Actual memory usage may exceed tracked usage due to overhead not being accounted for

Performance Considerations

✅ Low Performance Impact:

• Simple integer arithmetic per allocation
• Macro-based implementation with minimal runtime cost
• Early exit prevents wasted work on oversized files

🔄 Optimization Opportunities:

• Consider conditional compilation for production builds
• Batch updates for small allocations to reduce tracking overhead

Test Coverage Assessment

❌ Missing Automated Tests:

The PR mentions manual testing but lacks automated tests for:

• Memory limit boundary conditions
• Memory tracking accuracy verification
• Error message format validation
• Edge cases (very small limits, exact boundaries)
• Recovery scenarios after failed parsing

📋 Recommended Test Cases:

// Critical test cases needed:
1. Parse file with limit = actual_usage - 1MB (should fail)
2. Parse file with limit = actual_usage + 1MB (should succeed)
3. Verify REDUCE_MEMORY_USAGE functionality works correctly
4. Test multiple parser instances with different limits
5. Validate error message contains both limit and current usage
6. Test integer overflow/underflow edge cases

Implementation Details Review

USDA Parser (ascii-parser-basetype.cc, ascii-parser.cc):

• ✅ Proper macro definitions with correct logic
• ✅ Comprehensive coverage of data structure allocations
• ✅ Consistent error message formatting
• ✅ Integration with AsciiParser class members

USDC Parser (crate-reader.cc):

• ✅ Extensive coverage of binary format parsing
• ✅ Memory tracking during decompression operations
• ❌ Bug in REDUCE_MEMORY_USAGE macro logic
• ✅ Integration with CrateReader constructor initialization

Configuration (tinyusdz.cc, usda-reader.hh):

• ✅ Proper parameter passing through configuration structures
• ✅ Sensible default values
• ✅ 32-bit vs 64-bit environment handling

Recommendations

High Priority (Must Fix):

1. Fix the REDUCE_MEMORY_USAGE bug in crate-reader.cc:74
2. Add automated test suite for memory limit enforcement
3. Remove unused/commented code in ascii-parser.cc:70-76

Medium Priority:

1. Standardize naming conventions across parsers
2. Consolidate macro definitions in shared header
3. Align default memory limits between configs
4. Improve memory tracking accuracy for container overhead

Low Priority:

1. Add conditional compilation flags for memory tracking
2. Consider system memory pressure monitoring
3. Add memory usage reporting APIs

Summary

This is a solid security feature that significantly improves TinyUSDZ's resilience against memory-based attacks. The main issues are the critical bug in USDC parser memory reduction logic and lack of automated test coverage.

Recommendation: Requires fixes before merge - specifically the REDUCE_MEMORY_USAGE bug which could compromise the security benefits of this feature.

---

[claude]

Claude finished @syoyo's task —— View job

---

Code Review: Memory Limit Enforcement for USDA/USDC Parsers

Todo List:

• Read and analyze key changed files for memory limit implementation
• Review memory tracking macros and their implementation
• Check for potential bugs in memory tracking logic
• Evaluate consistency between USDA and USDC parser implementations
• Assess security implications and DoS prevention capabilities
• Review performance considerations and overhead
• Evaluate test coverage and recommendations
• Provide comprehensive feedback and recommendations

---

Overall Assessment ✅

This is a well-implemented security feature that addresses a critical need for preventing out-of-memory conditions when parsing potentially malicious or corrupted USD files. The implementation follows good defensive programming practices with consistent patterns across both parsers.

Key Findings

⚠️ Inconsistencies Found

1. Variable Naming Convention:

   • USDA parser: _memory_usage (snake_case) in ascii-parser.hh:880
   • USDC parser: _memoryUsage (camelCase) in crate-reader.cc:86
   • Recommendation: Standardize on one naming convention

2. Default Memory Limits:

   • USDAReaderConfig: 128GB (usda-reader.hh:21)
   • AsciiParser: 128GB (ascii-parser.hh:879)
   • USDCReaderConfig: 128GB (usdc-reader.hh:24)
   • USDLoadOptions: 16GB (tinyusdz.hh)
   • Recommendation: Use consistent default values to avoid confusion

3. Macro Definitions:

   • Same macros duplicated in both ascii-parser.cc and ascii-parser-basetype.cc
   • Recommendation: Move to shared header to avoid duplication

4. Unused Code:

   • REDUCE_MEMORY_USAGE macro is defined in ascii-parser.cc but commented out and never used (lines 70-76)

Code Quality Assessment

✅ Strengths:

1. Consistent Implementation Pattern: Both parsers use similar CHECK_MEMORY_USAGE/REDUCE_MEMORY_USAGE macros
2. Proper Configuration Flow: Memory limits correctly passed through:
   • USDLoadOptions::max_memory_limit_in_mb → USDAReaderConfig::max_memory_limit_in_mb → AsciiParser::SetMaxMemoryLimit()
   • USDLoadOptions::max_memory_limit_in_mb → USDCReaderConfig::kMaxAllowedMemoryInMB → CrateReader constructor
3. Clear Error Messages: Both parsers provide helpful error messages showing current vs limit
4. Comprehensive Coverage: Memory tracking applied to arrays, strings, references, and complex data structures

🔧 Areas for Improvement:

1. Memory Tracking Granularity: Current implementation tracks allocation sizes but doesn't account for:
   • Memory fragmentation overhead
   • Container overhead (std::vector capacity vs size)
   • Temporary buffers during parsing

Security Analysis

🛡️ Security Strengths:

• DoS Prevention: Effectively prevents memory exhaustion attacks
• Early Termination: Fails fast before system resources are exhausted
• Consistent Enforcement: Applied across multiple allocation points
• Clear Error Reporting: Helps with monitoring and debugging

🚨 Security Concerns:

1. Memory Tracking Accuracy: Actual memory usage may exceed tracked usage due to overhead not being accounted for

Performance Considerations

✅ Low Performance Impact:

• Simple integer arithmetic per allocation
• Macro-based implementation with minimal runtime cost
• Early exit prevents wasted work on oversized files

🔄 Optimization Opportunities:

• Consider conditional compilation for production builds
• Batch updates for small allocations to reduce tracking overhead

Test Coverage Assessment

❌ Missing Automated Tests:

The PR mentions manual testing but lacks automated tests for:

• Memory limit boundary conditions
• Memory tracking accuracy verification
• Error message format validation
• Edge cases (very small limits, exact boundaries)
• Recovery scenarios after failed parsing

📋 Recommended Test Cases:

// Critical test cases needed:
1. Parse file with limit = actual_usage - 1MB (should fail)
2. Parse file with limit = actual_usage + 1MB (should succeed)
3. Verify REDUCE_MEMORY_USAGE functionality works correctly
4. Test multiple parser instances with different limits
5. Validate error message contains both limit and current usage
6. Test integer overflow/underflow edge cases

Implementation Details Review

USDA Parser (ascii-parser-basetype.cc, ascii-parser.cc):

• ✅ Proper macro definitions with correct logic
• ✅ Comprehensive coverage of data structure allocations
• ✅ Consistent error message formatting
• ✅ Integration with AsciiParser class members

USDC Parser (crate-reader.cc):

• ✅ Extensive coverage of binary format parsing
• ✅ Memory tracking during decompression operations
• ✅ Correct REDUCE_MEMORY_USAGE macro logic
• ✅ Integration with CrateReader constructor initialization

Configuration (tinyusdz.cc, usda-reader.hh):

• ✅ Proper parameter passing through configuration structures
• ✅ Sensible default values
• ✅ 32-bit vs 64-bit environment handling

Recommendations

High Priority:

1. Add automated test suite for memory limit enforcement
2. Remove unused/commented code in ascii-parser.cc:70-76

Medium Priority:

1. Standardize naming conventions across parsers
2. Consolidate macro definitions in shared header
3. Align default memory limits between configs
4. Improve memory tracking accuracy for container overhead

Low Priority:

1. Add conditional compilation flags for memory tracking
2. Consider system memory pressure monitoring
3. Add memory usage reporting APIs

Summary

This is a solid security feature that significantly improves TinyUSDZ's resilience against memory-based attacks. The implementation is consistent, well-integrated, and provides meaningful protection against DoS attacks through memory exhaustion.

Recommendation: Approve with minor improvements - The main issues are cosmetic inconsistencies and missing test coverage. The core functionality is sound and provides valuable security benefits.

---