Heap buffer overflow read via signed char overflow in pattern counter (CWE-125/CWE-190)

[ByamB4]

Summary

A heap buffer overflow read vulnerability exists in pocketmod.h caused by a signed integer overflow of the pattern field in pocketmod_context. When a MOD file has exactly 128 patterns in the order table (length == 128), the signed char pattern field overflows from 127 to -128 when incrementing, bypassing the bounds check. This leads to out-of-bounds memory reads of approximately 130KB past the input buffer.

Root Cause

In struct pocketmod_context (line 84):

signed char pattern;        /* Current pattern in order */

The pattern field is declared as signed char, which has a range of -128 to 127. However, length is unsigned char (line 63) and can hold values up to 128 (valid per MOD format).

In _pocketmod_next_line() (line 271-276):

if (++c->line == 64) {
    if (++c->pattern == c->length) {
        c->pattern = c->reset;
    }
    c->line = 0;
}

When c->pattern == 127 and c->length == 128:

1. ++c->pattern overflows signed char from 127 → -128 (undefined behavior per C standard, but deterministic on all modern 2's complement systems)
2. The comparison (int)(-128) == (int)(128) is false due to integer promotion rules
3. c->pattern remains at -128, bypassing the reset to c->reset

Impact

1. Out-of-bounds read from order table (line 279)

pos = (c->order[c->pattern] * 64 + c->line) * c->num_channels * 4;

c->order[-128] reads 128 bytes before the order table. For 31-sample MODs, this reads from sample metadata (data[824]), producing an attacker-controlled pattern index (0-255).

2. Out-of-bounds write to visited bitmap (line 267)

c->visited[c->pattern >> 3] |= 1 << (c->pattern & 7);

c->visited[-16] writes to memory 16 bytes before the visited[] array, corrupting adjacent context struct fields (timing/rate variables).

3. Massive heap buffer overflow read of pattern data (line 280)

data = (unsigned char(*)[4]) (c->patterns + pos);

With an attacker-controlled pattern index P from the OOB order table read, pos can reach up to (255 * 64 + 63) * num_channels * 4. For a 4-channel MOD: pos = 261,328, causing a read approximately 130KB past the end of the input buffer.

The OOB data is decoded as pattern commands and ultimately mixed into the audio output buffer, potentially leaking heap memory contents through the audio stream (information disclosure).

Reproduction

1. Craft a 31-sample MOD file with data[950] = 128 (song length = 128 patterns)
2. Populate the order table (data[952..1079]) with valid pattern indices
3. Provide enough pattern data for all 128 patterns to pass pocketmod_init() validation
4. Call pocketmod_init() — succeeds because length == 128 passes the length > 128 check
5. Call pocketmod_render() in a loop. After processing all 128 patterns (128 render calls with default settings), the pattern counter overflows
6. The 129th pocketmod_render() call triggers OOB reads from heap memory

Suggested Fix

Change the type of pattern from signed char to int (or at minimum short):

 struct pocketmod_context
 {
     /* ... */
-    signed char pattern;        /* Current pattern in order                */
-    signed char line;           /* Current line in pattern                 */
+    int pattern;                /* Current pattern in order                */
+    int line;                   /* Current line in pattern                 */
     short tick;                 /* Current tick in line                    */
     float sample;               /* Current sample in tick                  */
 };

Alternatively, change the comparison to use >=:

-    if (++c->pattern == c->length) {
+    if (++c->pattern >= c->length) {

Environment

• pocketmod version: latest (commit from main branch, Feb 2026)
• Discovered via: manual source code audit
• CWE: CWE-125 (Out-of-bounds Read), CWE-190 (Integer Overflow or Wraparound)
• Severity: Medium (CVSS ~6.5 — requires user to open crafted MOD file and render audio)

[nikku4211]

Why change it from signed char to int instead of unsigned char?
Is there any reason why the pattern and line fields still need to be signed at all?
EDIT: Looked at the code, apparently -1 is used for initialising the lines and patterns.