Canon cleanup by rene-dev · Pull Request #4748 · LinuxCNC/linuxcnc

rene-dev · 2026-10-29T22:10:36Z

canon/interp: std::string and std::string_view instead of fixed char buffers

This branch cleans up the canon interface and the interpreter's text handling. Most fixed-size char buffers that were passed in or out of the interpreter become std::string or std::string_view. Each commit stands alone and builds on its own.

Along the way it fixes a number of buffer-overflow and truncation bugs, and removes several pieces of process-wide state that stood in the way of running more than one interpreter in a process.

Bugs fixed

Memory safety

Stack overflow in comment parameter expansion. convert_param_comment() wrote into a 513-byte buffer without ever checking the length. A (DEBUG,…), (PRINT,…), (LOG,…) or (ABORT,…) comment with many parameters could overflow it: 120 copies of #1 expand to about 1600 characters. The text is now built in a std::string. (10fb59dd59)
Unbounded strcpy of the error text. One of the two places in interp_convert.cc that save the current error before CHKS overwrites it copied it into a fixed buffer with strcpy. Both now copy into a std::string. (0a6d48b403)
Bare strcpy into the caller's buffer in find_ngc_file(). The found path was copied out with no length check. It is now returned as a std::string. (a0cee6bbcf)
Uninitialised buffer printed in an error message. When a subroutine file was not found, control_back_to() printed newFileName before anything had been written to it. It now prints the <name>.ngc it looked for. (a0cee6bbcf)
Uninitialised wordexp() result. find_ngc_file() used we_wordv[0] without checking that wordexp() succeeded. It now falls back to the unexpanded path. (a0cee6bbcf)
Memory leak. find_ngc_file() called wordexp() up to five times but wordfree() only once. (a0cee6bbcf)
Crash on an empty REMAP= line. parse_remap() passed NULL to strstore(), which throws. It now reports an error. (10fb59dd59)

Wrong results, truncation and silent limits

Long subroutine-path entries cut short. An INI value longer than PATH_MAX was silently truncated before being split. (a0cee6bbcf)
Long REMAP lines cut short. A REMAP line was truncated at LINELEN. (10fb59dd59)
REMAP options dropped. Options after the 19th were silently ignored. There is no limit now. (10fb59dd59)
Empty PROGRAM_PREFIX. It was searched as (null)/name.ngc. That step is now skipped. (a0cee6bbcf)
Half-resolved directories. When realpath() failed, glibc could leave a partly resolved path in program_prefix / wizard_root, and the subroutine search then looked in a directory that does not exist. The name is now empty. (91dca07a20)
Long parameter file names. GET_EXTERNAL_PARAMETER_FILE_NAME returned an empty name when the name didn't fit PARAMETER_FILE_NAME_LENGTH, and the INI reader rejected a long [RS274NGC]PARAMETER_FILE. There is no limit now. (c5735e013c)
Long file names in the task status. file_name() returned an empty string for a name longer than the buffer, so the task status showed no file. It is now truncated to fit the NML field instead. (749c04e28f)
Error text limit. The saved error text was limited to LINELEN. (0a6d48b403)
emccanon.GET_EXTERNAL_PARAMETER_FILE_NAME() from Python. It couldn't be called at all, because it wanted a char buffer. It now returns the name. (c5735e013c)
Misleading "too many entries" message. The SUBROUTINE_PATH message fired when exactly the maximum number of entries was given. It now fires only when there really is an extra entry. (a0cee6bbcf)

Interface hygiene

canon.hh: three NURBS helpers were declared twice, and alpha_finder was declared but never defined. (9dc04e27c6)
setup.log_file: a char[PATH_MAX] that was never read or written. (260aa2859f)
Canon text calls: MESSAGE, LOG, LOGOPEN and LOGAPPEND took a non-const char*, so callers with const strings needed casts. (c5735e013c)

Steps towards multiple interpreter instances (see nml discussion)

Error text belongs to the Interp. It moves from a file-scope static char savedError[] in the interpreter to the member Interp::saved_error_, so two interpreters no longer overwrite each other's errors. (0a6d48b403)
No more process-wide result buffers. The InterpBase getters (error_text, line_text, file_name, stack_name, command, file) return std::string, so callers no longer need buffers of their own. The globals this removes:
- interp_error_text_buf and interp_stack_buf in emctask.cc
- the static savedError in gcodemodule.cc
- the result buffers in sai
Canon interface narrowed to real canonical commands.
- 27 per-axis position, probe and tool-offset queries become three calls that return whole poses. (3aaef5bbad)
- The NURBS math helpers move out to nurbs_math.hh. (9dc04e27c6)
A smaller interface is less to turn into a per-instance object later.
Canon calls exchange values instead of shared buffers. Text goes in as std::string_view, and the parameter file name comes back as a std::string instead of being written into a buffer the caller owns. That is the shape needed for canon calls on an object. (c5735e013c)
Paths kept per instance. program_prefix, wizard_root and all the temporary path buffers are now std::string, so there are no PATH_MAX arrays in the setup struct apart from filename. (91dca07a20, a0cee6bbcf)
Plugin interpreters. interp_from_shlib() builds the plugin path without a fixed buffer. (a0cee6bbcf)

Still shared process-wide, not addressed here:

the static statustext in rs274ngc_pre.cc
the qc() queue statics
last_valid_heading
the USER_DEFINED_FUNCTION table
_task
the global canon state in emccanon, including the log and probe files

The nurbs_* functions evaluate G5.2/G6.2 curves. They are plain math used by the canon implementations, not canonical commands, so they do not belong in the interface the interpreter calls. Drop the three declarations that were listed twice, and alpha_finder, which was declared but never defined.

GET_EXTERNAL_POSITION_*, GET_EXTERNAL_PROBE_POSITION_* and GET_EXTERNAL_TOOL_LENGTH_*OFFSET were nine functions each, one per axis. The interpreter always reads all nine together, and emccanon already built the whole pose for every single-axis call. Replace the 27 functions with CANON_POSITION GET_EXTERNAL_POSITION(); CANON_POSITION GET_EXTERNAL_PROBE_POSITION(); EmcPose GET_EXTERNAL_TOOL_LENGTH_OFFSET(); so the interpreter makes one call where it made nine. The emccanon Python module keeps the per-axis names as wrappers, so remaps that call them, like stdglue.py, keep working.

MESSAGE, LOG, LOGOPEN and LOGAPPEND took a non-const char*, so callers holding a const string needed a cast. COMMENT and SET_PARAMETER_FILE_NAME took const char*. All six now take std::string_view, and the code behind them works on the view instead of copying it into a char buffer: - emccanon parses its hot comments (RPY, PROBEOPEN, PROBECLOSE) with string_view and writes the log and probe files through std::ofstream. setString/addString, used only there, are gone. - the preview renderer's comment hook takes the view, and the Python forwards pass it on as is. GET_EXTERNAL_PARAMETER_FILE_NAME filled a caller's buffer and returned an empty name when the name did not fit. It now returns a std::string, so PARAMETER_FILE_NAME_LENGTH is gone and the name has no length limit: the INI reader no longer rejects a long [RS274NGC]PARAMETER_FILE, and sai keeps the name in a std::string. From Python it was not callable at all (it wanted a char buffer) and now returns the name. CANON_ERROR was printf-variadic, but no caller passes arguments; it now takes the message as std::string_view. Callers that need formatting can use fmt::format. std::format is not an option while bookworm's GCC 12 is supported. The emccanon Python module gets small wrappers, since Boost.Python cannot convert to std::string_view.

PRINT and ECHO_WITH_ARGS wrapped fprintf, so a std::string_view had to be passed as "%.*s" with a separate length and pointer. They now use fmt::print, which takes a string_view as one argument and checks every format string at compile time. The UNEXPECTED diagnostics move to fmt::print as well. The output is unchanged: rs274 -g prints the same bytes as before for all 651 .ngc files under tests/ and nc_files/. START_SPEED_FEED_SYNC prints its bool with {:d}, because {} would print "true" where %d printed "1". rs274 already links -lfmt.

The interpreter logs through Interp::log_file, a FILE*. The char array of the same name in the setup struct was never read or written.

Both were PATH_MAX char arrays filled by realpath() into the buffer. They are now std::string, filled through a small real_path() helper that lets realpath() allocate the result. When realpath() fails the name is now empty. Before, glibc could leave the part of the path it had resolved in the buffer, and the subroutine search then looked in a directory that does not exist. findFile() only reads its directory and target, so they are const now.

The text of the last error lived in a file-scope static char buffer, so every Interp in a process shared it. It is now the member saved_error_, which also drops the LINELEN limit on its length. getSavedError() returns a const std::string&, and setSavedError() takes a std::string_view. The two callers that copied the text into a fixed buffer before CHKS overwrote it now copy it into a std::string; one of them used an unbounded strcpy.

error_text, line_text, file_name, stack_name, command and file each filled a caller's buffer and returned it. They now return a std::string. The buffers that existed only for these calls are gone: two globals in emctask.cc, the static savedError in gcodemodule.cc and the ones in sai. canterp keeps its file name in a std::string. file_name() returned an empty name when the name did not fit the buffer. It now returns the whole name, so for a name longer than the NML status field emctask now reports it truncated instead of empty. This changes the InterpBase interface, so an interpreter loaded with [TASK]INTERPRETER has to be rebuilt against the new header. rs274 -g prints the same output as before for all 651 .ngc files under tests/ and nc_files/, four of which end in an interpreter error.

…ffers find_ngc_file() and findFile() now build their candidate paths with std::string and hand the result back as a std::string, so there is no length limit and no bare strcpy into the caller's buffer. The repeated wordexp() calls go through a small expand_path() helper that frees each result; before, only the last one was freed. A wordexp() failure now falls back to the unexpanded path instead of reading an uninitialised result, and an empty PROGRAM_PREFIX is skipped rather than searched as "(null)/name.ngc". SUBROUTINE_PATH is split with string_view and resolved with real_path(), so a long value is no longer silently truncated. Empty entries are still skipped, and the "too many entries" message now only fires when there really is an extra entry. control_back_to() printed an uninitialised buffer as the file name when a subroutine file was not found; it now prints the <name>.ngc it looked for. save_parameters() reads the old .var file with std::getline and writes lines with fprintf, instead of going through a PATH_MAX-sized line buffer. A missing file still reads as empty, without the /dev/null fallback. interp_from_shlib() builds the plugin path as a std::string.

…mment expansion The error call stack (_setup.stack) only ever holds __PRETTY_FUNCTION__ or "Python", which are string literals, so it now keeps the pointers instead of copying each name into a 256-byte row. The ERM/ERS/ERN macros share an ERROR_STACK_START() helper. stack_name() and the Python error stack stop at the nullptr entry, and the Python side checks the array bound, since stack_index is writable from Python. parse_remap() splits the REMAP line into std::strings instead of copying it into a LINELEN buffer and running strtok_r on it, so a long line is no longer cut short. The limit of 19 options is gone; extra options used to be dropped silently. An empty REMAP line now reports an error instead of passing NULL to strstore(), which threw. convert_param_comment() appends to a std::string. It used to write into a 513-byte buffer without checking the length, so a (DEBUG,...) comment with many parameters could overflow the stack; 120 copies of #1 expand to about 1600 characters. Values longer than VAL_LEN still print as ######. write_state_tag() copies the file name into the fixed-size state_tag_t field with string_view::copy, as that struct is part of a motion message.

convert_remapped_code() built "O <name> call [v1][v2]..." in a char cmd[LINELEN], and add_parameters() strcat'ed each positional argument onto it without a length check. A remap with an '@' argspec and large word values, e.g. M400 P[10**300] Q[10**300], overflowed the stack buffer and crashed. The line is now built in a std::string. A line that is too long is rejected by read() with "Command too long" instead.

grandixximo · 2026-10-30T05:27:39Z

This looks like a nice cleanup, why are you keeping it as draft?

rene-dev · 2026-10-30T06:59:33Z

One test kept failing on my local machine, but it seems unrelated to this change, needs different investigation. Also, one more change incoming.

BsAtHome

This is just a quick bird-view scan through the changes.

BsAtHome · 2026-10-30T11:14:26Z

+extern std::vector<unsigned int> nurbs_G5_knot_vector_creator(unsigned int n, unsigned int k);
+


We should get rid of the extern here. It hasn't been necessary since ages on prototypes.

BsAtHome · 2026-10-30T14:44:46Z

-  if (filename[0] == 0)
-    rs274ngc_strxcpy(filename, RS274NGC_PARAMETER_FILE_NAME_DEFAULT);
-  CHP(restore_parameters(filename));
+  CHP(restore_parameters(parameter_file().c_str()));


Wouldn't it be better to also change Interp::restore_parameters() to take a const std::string & or a std::string_view as argument?

BsAtHome · 2026-10-30T14:54:54Z

+    if (wordexp(path.c_str(), &exp_result, 0) == 0) {
+        if (exp_result.we_wordc > 0)
+            out = exp_result.we_wordv[0];
+        wordfree(&exp_result);


Is the wordfree() call only necessary if wordexp() succeeds or may there be remnants left on failure? The man-page does not say explicitly, but the shown example doesn't check the return value and always calls wordfree().

BsAtHome · 2026-10-30T15:02:35Z

-	log_error("interp_error: {}\n", interp_error_text_buf);
+    const std::string error_text = interp.error_text(retval);
+    if (!error_text.empty()) {
+	log_error("interp_error: {}\n", error_text);


When touching this, you should fix the indent of the touched lines too.

rene-dev added 11 commits October 29, 2026 00:14

interp: remove the unused setup.log_file

260aa28

The interpreter logs through Interp::log_file, a FILE*. The char array of the same name in the setup struct was never read or written.

rene-dev marked this pull request as ready for review October 30, 2026 12:10

BsAtHome reviewed Oct 30, 2026

View reviewed changes

Navigation Menu

Repository navigation

Canon cleanup - #4748

Canon cleanup#4748
rene-dev wants to merge 11 commits into
LinuxCNC:masterfrom
rene-dev:canon-cleanup

rene-dev commented Oct 29, 2026

Uh oh!

grandixximo commented Oct 30, 2026

Uh oh!

rene-dev commented Oct 30, 2026

Uh oh!

BsAtHome left a comment

Uh oh!

BsAtHome Oct 30, 2026

Uh oh!

BsAtHome Oct 30, 2026

Uh oh!

BsAtHome Oct 30, 2026

Uh oh!

BsAtHome Oct 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		extern std::vector<unsigned int> nurbs_G5_knot_vector_creator(unsigned int n, unsigned int k);

Conversation

rene-dev commented Oct 29, 2026

canon/interp: std::string and std::string_view instead of fixed char buffers

Bugs fixed

Steps towards multiple interpreter instances (see nml discussion)

Uh oh!

grandixximo commented Oct 30, 2026

Uh oh!

rene-dev commented Oct 30, 2026

Uh oh!

BsAtHome left a comment

Choose a reason for hiding this comment

Uh oh!

BsAtHome Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

BsAtHome Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

BsAtHome Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

BsAtHome Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants