Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid null pointer dereference for partial configuration settings #13885

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RyanHeule
Copy link

Fixes #13864.

I first looked at how other extruder configuration options were validated and/or normalized. I found them at:

clamp_exturder_to_default(config.support_material_extruder, num_extruders);
clamp_exturder_to_default(config.support_material_interface_extruder, num_extruders);

and:
clamp_exturder_to_default(config.infill_extruder, num_extruders);
clamp_exturder_to_default(config.perimeter_extruder, num_extruders);
clamp_exturder_to_default(config.solid_infill_extruder, num_extruders);

Upon further analysis, I determined that wipe_tower_extruder validation does not really fit in either spot -- as far as I can tell, the wipe_tower_extruder can only be set at the print level, whereas the others can be set for individual objects (for supports) or even regions of an object (for the rest).

I considered adding another call of clamp_exturder_to_default [sic] in several other locations, such as within Print::validate(std::vector<std::string>*) or Print::apply(const Model &, DynamicPrintConfig). Quite possibly one of these spots could have worked, however due to my unfamiliarity with the code base, I was concerned that I may accidentally introduce other bugs. I also felt a large amount of testing would be required to validate such a change. For instance, I was not sure at what point it is no longer safe to change configuration (i.e. updating wipe_tower_extruder to a valid value) due to other logic already relying on it.

After a lot of deliberation, it seemed to me that DynamicPrintConfig::normalize_fdm() could work after all (despite my initial concerns expressed in #13864). I examined where it is called, and I found it is used in several stages of processing configuration -- for example, for each file provided by a --load command line option, for presets, for default FFF or SLA print options, and so on.

Even though early calls to DynamicPrintConfig::normalize_fdm() do not necessarily have full configuration (i.e. the original bug), for sure later calls do. And it seemed to me that someone had already tested wipe_tower_extruder validation at this location (just not without nozzle_diameter in the same file) to prove that this could work.

I did build this and do some very basic testing (a few command line calls, including what I was originally doing when I encountered the segmentation fault in the first place).

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.

Segmentation fault in Slic3r::DynamicPrintConfig::normalize_fdm
1 participant