FIX: null deref in ConfigBase::load_from_json when a project config has a malformed array

[ocidburn]

Title

FIX: null deref in ConfigBase::load_from_json when a project config has a malformed array

Body

ConfigBase::load_from_json dereferences a null option when a 3MF project config contains a
malformed array. One word fixes it, and it makes the call consistent with the three around it.

src/libslic3r/Config.cpp:1092-1096:

if (is_project_settings) {
    std::vector<std::string>& different_settings =
        this->option<ConfigOptionStrings>("different_settings_to_system", true)->values;
    size_t size = different_settings.size();
    if (size == 0) {
        size = this->option<ConfigOptionStrings>("filament_settings_id")->values.size() + 2;

Line 1096 is the only option<> call in this function that is dereferenced without a guard. Of the
six in load_from_json, lines 1078, 1083, 1088 and 1093 all pass the create-if-missing true, and
line 1062 assigns to a pointer that is then null-checked (if (diff_opt)). Only 1096 does neither,
so option() returns nullptr and ->values dereferences null.

How the key goes missing

The parse loop abandons the rest of the document on the first malformed array,
src/libslic3r/Config.cpp:1037-1041:

valid = parse_str_arr(it, single_sep, array_sep, escape_string_type, value_str);
if (!valid) {
    BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": parse " << file
                             << " error, invalid json array for " << it.key();
    break;
}

nlohmann iterates in sorted key order. In the file I looked at, extruder is key 86 of 478 and
filament_settings_id is key 128, so the break leaves 392 keys unparsed — filament_settings_id
among them. The block above then reads it unconditionally.

The file is a .3mf saved by Creality Print 5.1.7. Its per-extruder options are scalars rather than
arrays — of the 20 I checked, 20 are scalars:

"nozzle_diameter":   "0.4",          // expected ["0.4"]
"extruder_offset":   "0x0",
"retraction_length": "0.8",
"z_hop_types":       "Slope Lift",
"extruder_colour":   "#FCE94F",

Its extruder key is a nested object array, which parse_str_arr also rejects ("we only support
2 depth array").

Why it survived this long

Reaching it needs three things at once:

1. a malformed array whose key sorts before filament_settings_id (here extruder, key 86
   against key 128),
2. no different_settings_to_system in the file, so size == 0 and the branch is taken,
3. is_project_settings.

Projects written by BambuStudio emit well-formed arrays, so the path is only reached with a file
from another slicer.

The fix

-                    size = this->option<ConfigOptionStrings>("filament_settings_id")->values.size() + 2;
+                    auto *filament_ids = this->option<ConfigOptionStrings>("filament_settings_id");
+                    size = (filament_ids ? filament_ids->values.size() : 0) + 2;

A null check rather than create-if-missing, per review: option<>(key, true) would clone the
definition's default ConfigOptionStrings { "" } (PrintConfig.cpp:3059) and insert the key, which
PresetBundle.cpp:1128-1133 uses as a presence test to classify a preset. The size it produces is
discarded either way — the loop below only writes different_settings[0], and
PresetBundle.cpp:3776 re-resizes to num_filaments + 2 from filament_colour.

Provenance

The line arrived already missing the true in a2431d796 (2023-02-14, lane.wei, "ENH: add logic to
convert hybrid(auto) to tree(auto) for old 3mf"). Both lines were added in the same hunk:

+    ... this->option<ConfigOptionStrings>("different_settings_to_system", true)->values;
+    size = this->option<ConfigOptionStrings>("filament_settings_id")->values.size() + 2;

so the missing argument looks like an oversight at the time rather than something intended.

Scope

I deliberately left the break at line 1040 alone. Turning it into continue would keep the rest of
a partially-bad config and would probably have prevented this crash by itself, but it is a real
behaviour change and felt like your call rather than something to fold into a null guard. Happy to
follow up with it if you want it.

What I have and have not verified

• The analysis above is read against bambulab/BambuStudio@master as it stands today.
• I have not built or run BambuStudio, and the crash dump I have is from a derivative project
  (OrcaSlicer), which carries this line verbatim: Exception Code: c0000005 ACCESS_VIOLATION,
  RAX = 0, and RCX = 0x66696C616D656E74 — ASCII "filament" — which is what pointed at this
  call in the first place.
• The originating report, with the project file and full logs, is
  Null deref in ConfigBase::load_from_json when a 3MF project config has a malformed array (Creality Print 5.1.7 file) OrcaSlicer/OrcaSlicer#15337

I can attach the reproducing .3mf here as well if that is useful.

[tonghao-bbl]

Hi @ocidburn a nullptr check is preferfed to create when not exist here
@lanewei120 How do you think?

[ocidburn]

Thanks — agreed, and checking it made the case stronger than style.

option<>(key, true) does not create an empty option: DynamicConfig::optptr (Config.cpp:1631)
calls create_default_option() (Config.cpp:299), which clones the definition's default, and
filament_settings_id's default is ConfigOptionStrings { "" } (PrintConfig.cpp:3059) — one entry.
So it would give size = 3 and, more to the point, insert the key into the config.

That insertion is not harmless: PresetBundle.cpp:1128-1133 selects the preset collection by
presence, else if (config.has("filament_settings_id")) collection = &filaments; — so a config that
never had the key would start being classified as a filament preset instead of being rejected.
PresetBundle.cpp:3750 also notes the key "sometimes is not generated", which is why num_filaments
comes from filament_colour there — absence is expected, so fabricating it hides something the rest
of the code relies on.

The size difference costs nothing either way: the loop below only writes different_settings[0], and
PresetBundle.cpp:3776 re-resizes to num_filaments + 2 from filament_colour regardless.

Updated:

if (size == 0) {
    auto *filament_ids = this->option<ConfigOptionStrings>("filament_settings_id");
    size = (filament_ids ? filament_ids->values.size() : 0) + 2;
    different_settings.resize(size);
}

[tonghao-bbl]

LGTM @lanewei120