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

Fixes issue #5979 - NULL pointer crash in ModelObject::split() #5991

Closed
wants to merge 1 commit into from

Conversation

combolek
Copy link
Contributor

@combolek combolek commented Feb 8, 2021

ModelObject::split() expects a non-NULL new_objects vector where it adds pointers to the new models resulting from the split. But in the CLI case the caller does not care about this and passes NULL which causes a crash. To fix the crash we could pass a dummy vector but it turns out that we actually have a use for the results because we should assign a unique name to each new model the same way as the GUI does. These names show up as comments in the gcode so this change makes the gcode produced by the GUI and the CLI more similar and diffable.

ModelObject::split() expects a non-NULL new_objects vector where it adds pointers to the new models resulting from the split. But in the CLI case the caller does not care about this and passes NULL which causes a crash. To fix the crash we could pass a dummy vector but it turns out that we actually have a use for the results because we should assign a unique name to each new model the same way as the GUI does. These names show up as comments in the gcode so this change makes the gcode produced by the GUI and the CLI more similar and diffable.
@lukasmatena
Copy link
Collaborator

Thanks, this seems to have been there for a long time.
Your code renames even models that cannot be really split and some bits of code are duplicated. Is there a reason why I could not do this: eaa80df (branch lm_pr5991)? Does it break something?

@combolek
Copy link
Contributor Author

combolek commented Feb 8, 2021

Yes, I agree your change is better. I was just trying to mimic the GUI. I don't know the code well enough to tell if it doesn't break anything.

lukasmatena pushed a commit that referenced this pull request Feb 10, 2021
ModelObject::split() expects a non-NULL new_objects vector where it adds pointers to the new models resulting from the split.
But in the CLI case the caller does not care about this and passes NULL which causes a crash. To fix the crash we could pass
a dummy vector but it turns out that we actually have a use for the results because we should assign a unique name to each
new model the same way as the GUI does. These names show up as comments in the gcode so this change makes the gcode produced
by the GUI and the CLI more similar and diffable.

@lukasmatena has amended the original commit by @combolek (pull request #5991) in order to avoid code duplication
@lukasmatena
Copy link
Collaborator

@combolek I have amended your commit with my proposed change and merged it into master: 22b2ccc. Thanks a lot.
Closing.

@combolek combolek deleted the issue-5979 branch February 10, 2021 16:30
@combolek combolek restored the issue-5979 branch February 10, 2021 16:31
@combolek combolek deleted the issue-5979 branch July 26, 2021 21:56
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.

2 participants