Skip to content

mappers: Implement mapper 11 - #21

Merged
rene merged 1 commit into
mainfrom
imp-mapper-11
Sep 20, 2026
Merged

rene merged 1 commit into
mainfrom
imp-mapper-11

Conversation

@rene

@rene rene commented Sep 20, 2026

Copy link
Copy Markdown
Owner

Implement mapper 11.

Implement Mapper 11 - Color Dreams.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
@rene

rene commented Sep 20, 2026

Copy link
Copy Markdown
Owner Author

@claude

@github-actions

github-actions Bot commented Sep 20, 2026 •

Copy link
Copy Markdown

Claude finished @rene's task —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review feedback

Overall the implementation is clean and consistent with the existing mapper pattern. I found a few issues worth addressing:

src/mappers/m11_ColorDreams.c

Bug: Out-of-bounds read during bus conflict emulation

m11_prg_mem_handler:147–156: When a write arrives, idx is computed as m11->prg_bank + (address - 0x8000). After the bank-switch completes (m11_update), prg_bank will point to the newly selected bank — but the bus-conflict AND uses the value before the update, so the current idx calculation is correct for the conflict read. However there is no bounds check on idx against prg_size before indexing into prg_rom. If the ROM image is malformed (e.g. prg_size is smaller than expected), this produces an out-of-bounds read:

// Before touching the register, guard the access:
if (idx >= cartridge->rom->prg_size)
    return 0;

The same applies to the CMEM_READ path on line 151.

Bug: No bounds check on CHR ROM access

m11_chr_mem_handler:182–191: idx = m11->chr_bank + (address & 0x1fff) is never validated against chr_size before indexing chr_rom. A malformed ROM with chr_size == 0 and no CHR RAM configured would dereference a NULL (or garbage) pointer:

if (idx >= cartridge->rom->chr_size)
    return 0;

Compare with m7_AxROM.c which has the same latent issue — but for a new mapper it would be good to fix it proactively here.

Nit: Missing SPDX-License-Identifier header

All other mapper files (m0_NROM.c, m3_CNROM.c, m7_AxROM.c, etc.) start with /* SPDX-License-Identifier: BSD-3-Clause */. This file is missing that line.

Nit: Redundant initializer

m11_prg_mem_handler:142: uint64_t idx = 0; — the = 0 is never read before idx is assigned on line 147. All other handlers declare uint64_t idx; without an initializer. Minor but inconsistent with the surrounding style.

Note: lockout field unused

The lockout : 2 bitfield in _m11_bank_reg is parsed but never acted upon. This is fine for correctness (real Color Dreams carts use it to lock the bank register), but it may be worth a comment noting the intentional omission if it is out of scope.

Note: prg_bank/chr_bank fields could be uint32_t

struct _m11_mapper uses uint64_t for prg_bank and chr_bank. The maximum PRG bank offset for mapper 11 is 3 * 0x8000 = 0x18000 and CHR is 15 * 0x2000 = 0x1E000, both of which fit easily in uint32_t. Other mappers (e.g. m3_CNROM.c, m7_AxROM.c) use uint64_t idx as a local, so uint64_t is not wrong, but being consistent with the struct member type vs. local variable type could be cleaner.


Summary: The two bounds-check issues (PRG and CHR ROM array accesses) are real bugs that could cause crashes with malformed/edge-case ROMs. The missing SPDX tag is a style inconsistency. Everything else is minor.

@rene
rene merged commit 66baaed into main Sep 20, 2026
4 checks passed
@rene
rene deleted the imp-mapper-11 branch September 27, 2026 14:39
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