C++: Accessing class member pointer from inside class changes value

Question

I have this function

uint8_t ROM::read(uint16_t addr)
{
    uint16_t mapped_addr = 0;
    mapper->CPUread(addr, mapped_addr);
    return prg_rom[mapped_addr];

}

inside my ROM class. When this function is called and accesses the CPUread function of the mapper. The mapper's values change. However, when call the mapper CPUread from outside the ROM class, it works as expected.

ROM rom("nestest.nes");
uint16_t mapped_addr = 0;
rom.mapper->CPUread(0xC010, mapped_addr);

Calling it like this(below) does not work, (EDIT: it assigns random values to prg_banks and chr_banks

ROM rom("nestest.nes");
rom.read(0xC010);

ROM.cpp

#include "ROM.h"

ROM::ROM(std::string filename)
{
    std::ifstream file;
    file.open(filename, std::ifstream::binary);

    if (file.is_open())
    {
        //Read and store nes file header
        file.read((char*)&header, sizeof(HEADER));

        //Determine mapper ID
        mapper_id = (header.flag_7 & 0xF0) | ((header.flag_6 & 0xF0) >> 4);

        //Determine if rom contains trainer
        if (header.flag_6 & 0x40)
        {
            file.seekg(512, std::ios_base::cur);
        }

        //Read and store prg rom data
        prg_rom = (uint8_t*)malloc(header.prg_blocks * 16384);
        file.read((char*)prg_rom, header.prg_blocks * 16384);

        //Read and store chr rom data
        chr_rom = (uint8_t*)malloc(header.chr_blocks * 8192);
        file.read((char*)chr_rom, header.chr_blocks * 8192);

        switch (mapper_id)
        {
        case 0x00:
            mapper = &Mapper_000(header.prg_blocks, header.chr_blocks);
            break;
        default:
            break;
        }
    }
    else
    {
        std::cout << "Could not open file: " << filename << std::endl;
    }

    file.close();
}

uint8_t ROM::read(uint16_t addr)
{
    uint16_t mapped_addr = 0;
    mapper->CPUread(addr, mapped_addr);
    return prg_rom[mapped_addr];

}

ROM.h

#pragma once
#include <cstdint>
#include <array>
#include <fstream>
#include <iostream>
#include <string>

#include "Mapper.h"
#include "Mapper_000.h"

class ROM
{
public:
    uint8_t* prg_rom = nullptr;
    uint8_t* chr_rom = nullptr;
    uint8_t mapper_id = 0;

    Mapper* mapper = nullptr;

    struct HEADER
    {
        char        constant[4];    //0-3: Constant $4E $45 $53 $1A ("NES" followed by MS-DOS end-of-file)
        uint8_t     prg_blocks;     //Size of PRG ROM in 16 KB units
        uint8_t     chr_blocks;     //Size of CHR ROM in 8 KB units (Value 0 means the board uses CHR RAM)
        uint8_t     flag_6;         //Mapper, mirroring, battery, trainer
        uint8_t     flag_7;         //Mapper, VS/Playchoice, NES 2.0
        uint8_t     flag_8;         //PRG-RAM size (rarely used extension)
        uint8_t     flag_9;         //TV system (rarely used extension)
        uint8_t     flag_10;        //TV system, PRG-RAM presence (unofficial, rarely used extension)
        uint8_t     unused[5];      //Unused padding (should be filled with zero, but some rippers put their name across bytes 7-15)
    } header;

public:
    ROM(std::string filename);

    uint8_t read(uint16_t addr);
};

Mapper.h

#pragma once
#include <cstdint>

class Mapper
{
protected:
    uint8_t prg_banks = 0;
    uint8_t chr_banks = 0;

public:
    Mapper(uint8_t prgBanks, uint8_t chrBanks)
    {
        prg_banks = prgBanks;
        chr_banks = chrBanks;
    }

    virtual void CPUread(uint16_t addr, uint16_t &mapped_addr) = 0;
    virtual void CPUwrite(uint16_t addr, uint16_t &mapped_addr) = 0;
};

Mapper_000.h

#pragma once
#include <cstdint>

#include "Mapper.h"

class Mapper_000 : public Mapper
{
public:
    //Constructor
    Mapper_000(uint8_t prgBanks, uint8_t chrBanks) : Mapper(prgBanks, chrBanks)
    {
    }

    //Read & write
    void CPUread(uint16_t addr, uint16_t& mapped_addr);
    void CPUwrite(uint16_t addr, uint16_t& mapped_addr);
};

Mapper_000.cpp

#include <cstdint>

#include "Mapper_000.h"


//Read & Write
void Mapper_000::CPUread(uint16_t addr, uint16_t& mapped_addr)
{
    if (addr >= 0x8000 && addr <= 0xFFFF)
    {
        if (prg_banks > 1)
        {
            mapped_addr = addr & 0x7FFF;
        }
        else
        {
            mapped_addr = addr & 0x3FFF;
        }
    }
}

void Mapper_000::CPUwrite(uint16_t addr, uint16_t& mapped_addr)
{
    if (addr >= 0x8000 && addr <= 0xFFFF)
    {
        if (prg_banks > 1)
        {
            mapped_addr = addr & 0x7FFF;
        }
        else
        {
            mapped_addr = addr & 0x3FFF;
        }
    }
}

Answer 1

In the ROM constructor, we find

mapper = &Mapper_000(header.prg_blocks, header.chr_blocks);

which takes the address of a Temporary variable. A temporary only exists until the end the statement that uses it, so immediately after assigning the pointer to it, this lovely new object goes out of scope, leaving mapper a dangling pointer.

Because mapper doesn't exist, calling CPUread on it is undefined, but doesn't really become noticeable until the program stores a value in its also-nonexistent mapped_addr member. Likely it's the extra function call involved in the real code that moves the stack around enough to reuse the storage of the now-dead object and make the error visible when it's not visible in the simple test case. Ain't UB wonderful?

My solution looks something like this:

ROM.h picks up an include of

#include <memory>

to get smart pointers to automatically manage the lifetime of a dynamically allocated Mapper. Note: This means Mapper's destructor must be virtual.

In the ROM class definition,

Mapper* mapper = nullptr;

becomes

std::unique_ptr<Mapper> mapper;

and the offending line in the constructor

mapper = &Mapper_000(header.prg_blocks, header.chr_blocks);

becomes

mapper = std::make_unique<Mapper_000>(header.prg_blocks, header.chr_blocks);

to dynamically allocate a Mapper_000 that will survive until the ROM object goes out of scope.

Documentation for std::unique_ptr

Note a temporary variable is an rvalue, and taking the address of an rvalue is supposed to be illegal. Likely Visual Studio's clients have millions of lines of old code that count on this remaining legal. Maybe there are Visual Studio-specific compiler exceptions that make this semi-legal in some cases. Maybe it's flagged as a warning or an error with the correct options enabled.