Implement fallback for corrupted nvram.vmp and similar files

[izzyreal]

For various reasons, files VMPC2000XL tries to load may become corrupted. At the moment, for nvram.vmp, and maybe some other files too, we only check if the file exists, and then assume it is healthy. For nvram.vmp in particular, see DefaultsParser::AllDefaultsFromFile. But we should try to find other places that make the same assumption as well, and then everywhere implement detection of corruption, and a fallback mechanism. In most cases, adding a file-size check on top of the file-exists check should suffice, so let's start with that. Additionally we may delete or rename the corrupted file, to ensure there's no attempt of loading it again.

Actual crash log submitted by Nino Beatz:

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Subtype: KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Codes: 0x0000000000000001, 0x0000000000000000
VM Region Info: 0 is not in any region.  Bytes before following region: 4378378240
      REGION TYPE                 START - END      [ VSIZE] PRT/MAX SHRMOD  REGION DETAIL
      UNUSED SPACE AT START
--->  
      __TEXT                   104f8c000-105370000 [ 3984K] r-x/r-x SM=COW  /var/containers/Bundle/Application/212A9F9C-42A4-4F58-A250-D21D109054F8/VMPC2000XL.app/PlugIns/VMPC2000XL.appex/VMPC2000XL
Termination Reason: SIGNAL 11 Segmentation fault: 11
Terminating Process: exc handler [521]

Triggered by Thread:  0

Thread 0 name:   Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   libsystem_platform.dylib      	       0x202abbab0 _platform_memmove + 144
1   VMPC2000XL                    	       0x105107728 mpc::Util::vecCopyOfRange(std::__1::vector<char, std::__1::allocator<char>> const&, int, int) + 88
2   VMPC2000XL                    	       0x10531e148 mpc::nvram::DefaultsParser::AllDefaultsFromFile(mpc::Mpc&, ghc::filesystem::path) + 56
3   VMPC2000XL                    	       0x1053229a8 mpc::nvram::NvRam::loadUserScreenValues(mpc::Mpc&) + 404
4   VMPC2000XL                    	       0x1050fd50c mpc::Mpc::init() + 6060

[izzyreal]

Inside ~/Application Support/VMPC2000XL/config we find:

• volumes.json
• midicontrolmapping.vmp
• nvram.vmp
• vmpc-specific.ini

Let's look at how prone these are to cause a crash when they have become corrupted.

volumes.json

We're using https://github.com/Tencent/rapidjson to read this file. This happens in VolumesPersistence.cpp, in freestanding function Document read(mpc::Mpc& mpc), which looks pretty fail-safe:

...
if (fs::exists(path))
{
    const auto bytes = get_file_data(path); 
    result.Parse(bytes.data());
}
if (!result.IsObject())
{
    result.SetObject();
}
if (!result.HasMember("volumes"))
{
    result.AddMember("volumes", Value().SetArray(), result.GetAllocator());
}
...

Now let's look at invokers of read.

std::string VolumesPersistence::getPersistedActiveUUID(mpc::Mpc& mpc)
{                  
    Document doc = read(mpc);
    Value& volumes = doc["volumes"];
                   
    for (auto i = volumes.Begin(); i != volumes.End(); i++)
    {              
        auto uuid = (*i)["uuid"].GetString();
        auto isActive = (*i)["active"].GetBool();
                   
        if (isActive)
            return uuid;
    }

    return "";
}

This is not safe. We must check for existence of keys:

• uuid
• active

Likewise for these keys in VolumesPersistence::getPersistedConfigs:

auto uuid = (*i)["uuid"].GetString();
persistedConfigs[std::string(uuid)] = (MountMode)(*i)["mode"].GetInt();

• uuid
• mode

And finally volumes and uuid (2 times) in VolumesPersistence::save:

• Value& volumes = d["volumes"];
• auto uuid = (*i)["uuid"].GetString();
• auto uuid = (*j)["uuid"].GetString();

midicontrolmapping.vmp

This file, and other MIDI control presets, are read via AbstractDisk::readMidiControlPreset. This method checks if the file has at least 681 bytes, so that's kind of ok. But we can make things a bit better and safer by making improvements to:

for (int i = 0; i < 16; i++)
{
    if (data[i + 1] == ' ') continue;
    presetName.push_back(data[i + 1]);
}

• Check if each pushed char is present in Mpc::akaiAsciiChar. If it's not, skip it.
• If an empty name is yielded, fall back to file name.

And we should also improve this part of the same method:

while (pointer < data.size())
{                        
    std::string name;    
    char c;              
                         
    while ((c = data[pointer++]) != ' ' && pointer < data.size())
    {                    
        name.push_back(c);
    }   
    
    const bool isNote = data[pointer++] == 1;
    const char channel = data[pointer++];
    const char value = data[pointer++];
    
    auto cmd = MidiControlCommand{name, isNote, channel, value};
    preset->rows.emplace_back(cmd);
}   

• For name, check if each pushed char is present in Mpc::akaiAsciiChar. If it's not, skip it.
• If an empty name is yielded, skip this entry.
• If the name is not an existing command, skip this entry.
• std::clamp channel and value.

nvram.vmp

NvRam::loadUserScreenValues only loads nvram.vmp if it has mpc::file::all::AllParser::DEFAULTS_LENGTH bytes.
For now this should cover most expected cases of corruption.

If someone really wants to, they could replace the file with arbitrary data, and it will bring VMPC2000XL into an invalid state, most likely resulting in cosmetic garbage at best, and crashing at worst. To avoid this, we could implement clamping in mpc::file::all::Defaults. I suppose we could do this, but I'm not sure if it warrants the effort at this point. Moreover, if we do clamping there, it would make sense to do it in all the components of all the formats that can be loaded. This seems beyond the scope of this issue, and might be better handled by relying on https://github.com/izzyreal/mpc2000xl_kaitai. These format descriptions allow to specify limits, which I've tried to do in many places already there. This way a file can be validated with relative ease, and rejected if it's corrupt.

So, for now we will leave loadUserScreenValues as is, assuming nobody goes out of their way to force something that's not really an nvram.vmp into nvram.vmp.

vmpc-specific.ini

NvRam::loadVmpcSettings already does a check each along each step of the way if the file size is big enough. This provides ok safety. We improve it a bit with some clamping:

• Clamp each parameter.