Skip to content

fix: prevent NULL FILE* crash when loading a missing G-code file - #648

Merged
LiuLikeQian merged 3 commits into
mainfrom
fix_gcodereader_null_deref
Aug 3, 2026
Merged

fix: prevent NULL FILE* crash when loading a missing G-code file#648
LiuLikeQian merged 3 commits into
mainfrom
fix_gcodereader_null_deref

Conversation

@iesteem

@iesteem iesteem commented Jul 30, 2026

Copy link
Copy Markdown

Summary

Two related defensive fixes for the G-code load path, split out of PR #589 (filament temp mixing) per change-scope-control review finding (M3):

  • GCodeReader::parse_file_raw_internal returns false when fopen fails, preventing ::fread(buffer.data(), 1, ..., NULL) from triggering a CRT invalid-parameter crash on Windows (and UB elsewhere).
  • Plater::load_gcode rejects a missing / inaccessible file up front with a precise "does not exist" message, instead of letting the load path proceed and surface the misleading "does not contain valid G-code" error.

Why split out

These were originally bundled into PR #589 commit 4d7ffd5 alongside the mixing-notification work. They have an independent regression surface (loading missing / invalid G-code files) and should ship in a separate PR so any regression can be bisected cleanly. The split was done as:

Test plan

  • Load a project, then attempt to load a G-code file whose path has been deleted between file-picker and load → expect the new "The selected file: ... does not exist." dialog (no CRT crash).
  • Load a valid G-code file → unchanged behavior, no regression.
  • CLI path: slice a project whose referenced G-code path is invalid → parse_file_raw_internal returns false cleanly (no CRT crash on Windows).
  • zh_CN locale: the new "does not exist." string renders correctly; other locales fall back to English until update-translation.yml syncs.

Downstream

This PR can merge independently of #589. After both merge, the final state of main is identical to what PR #589 would have produced if the GCodeReader/load_gcode changes had not been bundled.

🤖 Generated with Claude Code

@iesteem @claude
fix: prevent NULL FILE* crash when loading a missing G-code file
84f5cbd 
Two related defensive fixes for the G-code load path:

- GCodeReader::parse_file_raw_internal now returns false when fopen
  fails, preventing ::fread(buffer.data(), 1, ..., NULL) from triggering
  a CRT invalid-parameter crash on Windows (and UB elsewhere).
- Plater::load_gcode rejects a missing / inaccessible file up front
  with a precise "does not exist" message, instead of letting the load
  path proceed and surface the misleading "does not contain valid
  G-code" error.

Originally bundled into PR #589 (filament temp mixing) commit 4d7ffd5;
split out per change-scope-control review finding (M3) so any
regression on the G-code load path can be bisected independently of
the mixing-notification changes.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@iesteem iesteem mentioned this pull request Jul 30, 2026
Merged
11 tasks
iesteem added 2 commits July 30, 2026 19:11
@iesteem
fix: prevent NULL FILE* crash when loading a missing G-code file
f399ca4 
Two related defensive fixes for the G-code load path:

- GCodeReader::parse_file_raw_internal now returns false when fopen
  fails, preventing ::fread(buffer.data(), 1, ..., NULL) from triggering
  a CRT invalid-parameter crash on Windows (and UB elsewhere).
- Plater::load_gcode rejects a missing / inaccessible file up front
  with a precise "does not exist" message, instead of letting the load
  path proceed and surface the misleading "does not contain valid
  G-code" error.

Originally bundled into PR #589 (filament temp mixing) commit 4d7ffd5;
split out per change-scope-control review finding (M3) so any
regression on the G-code load path can be bisected independently of
the mixing-notification changes.
@iesteem
Merge branch 'fix_gcodereader_null_deref' of https://github.com/Snapm…
87c9de4 
…aker/OrcaSlicer into fix_gcodereader_null_deref
@LiuLikeQian
LiuLikeQian merged commit 24593a3 into main Aug 3, 2026
16 checks passed
iesteem added a commit that referenced this pull request Aug 4, 2026
@iesteem @claude
Merge branch 'main' into fix_top_cover
a184e77 
Brings PR #627 (cold-plate gating), #648 (G-code null FILE* fix), and
#626 (flow_ratio_zero blocker) into fix_top_cover, alongside the
already-present PR #589 work (filament temp mixing + cached rendering).

Conflict resolutions:
- Plater.hpp: adopt main's FlowRatioZeroDetail / ColdPlateCompatState /
  ColdPlateCompatResult (HEAD side had no equivalents).
- Plater.cpp (26 conflicts):
  * Drop main's duplicate filament_display_label / format_filament_slot_list;
    keep HEAD's Allman versions, but route through main's new tr_u8() helper
    (fixes I18N::translate_utf8 dangling-pointer trap).
  * Keep HEAD's mixing preference label "Allow high/low temperature filament
    mixing" (matches actual Preferences.cpp:1247 setting text); main's
    older "Allow mixed printing ..." wording was the stale version.
  * Keep HEAD's Allman brace style and unicode "->"/"—" comment characters
    per P-tier coding standard.
  * Keep HEAD's sync_filament_temp_mixing_notification structure (close-old +
    cache refresh at top of function); drop main's mid-function close-old
    duplicate.
  * Keep HEAD's AllowedWarning semantics: cached text excludes the WARNING:
    prefix so close_validate_warning_notification's prefix matching works.
  * Adopt main's flow_ratio_zero_error_text and cold_plate_*_text additions
    unchanged.
- GLCanvas3D.cpp / Plater.cpp Preview-tab entry: collapse to single
  is_plate_sliceable(i) gate from main.
- is_plate_sliceable(): swap mixing check to the cached variant
  (is_plate_blocked_by_filament_temp_mixing_cached) so the GLCanvas3D hot
  rendering path no longer recomputes check_filament_temp_mixing per plate
  per frame. This is the PR #589 perf win landing inside the unified gate.
- zh_CN.po: keep HEAD's mixing preference msgid; keep main's "does not
  exist." once (drop the duplicated copy).

Compile-verified: libslic3r, libslic3r_gui, Snapmaker_Orca.dll/exe all link
(0 errors, only pre-existing C4101/C4996/LNK warnings).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
iesteem added a commit that referenced this pull request Aug 4, 2026
@iesteem
fix: harden tr_u8 with null defenses and correct the rationale comment
8f487a6 
The previous comment claimed tr_u8 works around a dangling-pointer trap
in I18N::translate_utf8. That's inaccurate — by C++ rules the temporaries
in wxGetTranslation(...).ToUTF8().data() live until the end of the full
expression, which includes the std::string copy construction. Rewrite the
comment to state the real reason (defensive against MSVC /O2 mis-optimization
of temporary chains, same pattern that bit us in #648).

Also add null defenses on both ends of the call:
- wxGetTranslation / wxString(nullptr, wxConvUTF8) are UB on null input.
- std::string(nullptr, 0) is UB even with zero length, so buf.data() must
  be checked before construction.

Returns "" on null input.
LiuLikeQian pushed a commit that referenced this pull request Aug 4, 2026
@iesteem @claude
fix: unify filament temp mixing blocking with plate error gating (#589)
38ef299 
* fix: unify filament temp mixing blocking with plate error gating

- Remove has_sliceable_plate_for_slice_all() hijack in Preview tab switch
  that triggered unintended "slice all" instead of switching to preview
- Add m_filament_temp_blocked flag to PartPlate so can_slice() reflects
  filament temp mixing state, consistent with m_apply_invalid for
  object-outside errors
- Let sync_filament_temp_mixing_notification() manage its own flag,
  independent of the background validation system
- Simplify EVT_GLVIEWTOOLBAR_PREVIEW to use can_slice() as unified gate
  for both error types

Downstream effects: red exclamation icon on plate thumbnails, disabled
slice/export buttons, and blocked auto-slice on preview switch now work
uniformly for both object-outside and filament-temp-mixing errors.

* fix: use on-demand checks instead of cached flag for filament temp mixing

Replace the per-plate cached m_filament_temp_blocked flag with on-demand
is_plate_blocked_by_filament_temp_mixing() checks in the rendering and
preview-switch paths. This eliminates staleness risks identified during
code review where non-current plates could retain stale blocked state
after filament changes, undo/redo, or plate switching during slicing.

Changes:
- GLCanvas3D: add on-demand check for red exclamation on plate thumbnails
- Plater: add on-demand check in EVT_GLVIEWTOOLBAR_PREVIEW alongside
  can_slice(), keep sync_filament_temp_mixing_notification() for
  notification freshness
- PartPlate: revert cached flag; can_slice() unchanged

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* refactor: simplify filament slot collection in check_filament_temp_mixing

Merge duplicate key lists, rename used_slots for clarity, and group logic
into scoped blocks. No semantic change.

* refactor: rename STRING_EXCEPT_FILAMENTS_DIFFERENT_TEMP to STRING_EXCEPT_FILAMENTS_MIXING_TEMP

Better reflects that the check is about high/low temperature material mixing,
not just "different" temperatures.

* feat: show per-filament high/low temp breakdown in mixing notifications

Add per-filament high/low temperature breakdown to the filament temp
mixing warning/error notifications so users can see exactly which slots
are in conflict, both for single-plate and slice-all modes.

- Plater::check_filament_temp_mixing now exposes a FilamentTempMixingDetail
  (high/low 1-based slot lists) via a new overload
- Notification text becomes dynamic; sync_filament_temp_mixing_notification
  caches the exact pushed strings to work around NotificationManager's
  exact-text close matching
- confirm_filament_temp_mixing_before_slice / _before_slice_all use the
  same breakdown in their modal dialogs
- Safe translation helper tr_u8() avoids the dangling-pointer trap in
  I18N::translate_utf8() that crashed in non-English locales
- Add zh_CN translations for the new strings

Also fix a separate crash: GCodeReader::parse_file_raw_internal no longer
dereferences a NULL FILE* when fopen fails (e.g. loading a non-existent
gcode file at startup), and Plater::load_gcode now checks wxFileExists
up front to give a precise "does not exist" error instead of the vague
"does not contain valid G-code".

* fix: address PR #589 review findings on filament temp mixing

- B1 perf: cache per-plate blocked state in Plater::priv and add
  is_plate_blocked_by_filament_temp_mixing_cached() for the per-frame
  plate-thumbnail render path. Cache is refreshed once per
  sync_filament_temp_mixing_notification() call, which already runs at
  every state-changing event, so hot reads are O(1).
- B2: close previously-pushed mixing notifications BEFORE the
  curr_plate==nullptr early return so the on-screen banner does not
  leak when the current plate is transiently null during a switch.
- B3: remove the tr_u8() helper. I18N::translate_utf8() returns
  std::string by value; the dangling-pointer narrative was incorrect.
  All call sites revert to _u8L().
- B4: warning notification close text mismatch. push_notification stores
  the body verbatim, but close_validate_warning_notification(text)
  compares against "WARNING:\n" + text. Cache the un-prefixed body so
  push/close text match exactly; the previous code never closed the
  warning banner.
- m1: restore Allman brace style in new helpers and check_filament_temp_mixing
  overload to match the rest of Plater.cpp (project coding standard 2.3).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* revert: move GCodeReader/load_gcode defensive fixes out of PR #589

These two unrelated defensive fixes were bundled into commit 4d7ffd5
alongside the mixing-notification work. Per the change-scope-control
review finding (M3), they have an independent regression surface
(loading missing / invalid G-code files) and should ship in a separate
PR so any regression can be bisected cleanly.

Reverted in this commit (will be re-applied on a separate branch):
- src/libslic3r/GCodeReader.cpp: NULL FILE* check in parse_file_raw_internal
- src/slic3r/GUI/Plater.cpp: wxFileExists early-reject in Plater::load_gcode
- localization/i18n/zh_CN/Snapmaker_Orca_zh_CN.po: "does not exist." key

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix: align mixing notification preference name with actual setting

The error/warning notification referenced the filament mixing
preference as "Allow mixed printing of high and low temperature
materials", which doesn't match the actual preference label
"Allow high/low temperature filament mixing". Update the
notification source literal and zh_CN translations to use the
canonical name so users can locate the preference the notification
points them to.

* fix: harden tr_u8 with null defenses and correct the rationale comment

The previous comment claimed tr_u8 works around a dangling-pointer trap
in I18N::translate_utf8. That's inaccurate — by C++ rules the temporaries
in wxGetTranslation(...).ToUTF8().data() live until the end of the full
expression, which includes the std::string copy construction. Rewrite the
comment to state the real reason (defensive against MSVC /O2 mis-optimization
of temporary chains, same pattern that bit us in #648).

Also add null defenses on both ends of the call:
- wxGetTranslation / wxString(nullptr, wxConvUTF8) are UB on null input.
- std::string(nullptr, 0) is UB even with zero length, so buf.data() must
  be checked before construction.

Returns "" on null input.

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LiuLikeQian LiuLikeQian LiuLikeQian approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iesteem @LiuLikeQian