sai: give rs274 its own tool mmap instead of truncating $HOME/.tool.mmap - #4385
Open
tzuohann wants to merge 2 commits into
Open
sai: give rs274 its own tool mmap instead of truncating $HOME/.tool.mmap#4385tzuohann wants to merge 2 commits into
tzuohann wants to merge 2 commits into
Conversation
BsAtHome
reviewed
Aug 16, 2026
grandixximo
reviewed
Aug 16, 2026
tzuohann
force-pushed
the
rs274-private-toolmmap
branch
from
August 16, 2026 14:44
3c37f31 to
74522c6
Compare
BsAtHome
reviewed
Aug 16, 2026
tool_mmap_creator() opens the file O_RDWR|O_CREAT|O_TRUNC
(tooldata_mmap.cc:33, used at :135) and sai calls it at driver.cc:570 --
before getopt() at :578. Every rs274 invocation therefore empties
$HOME/.tool.mmap, including `rs274 --help` and including one that supplies
-t, since -t is not read until :583.
That file is not scratch space: tool_mmap_fname() builds it from
secure_getenv("HOME") with a fixed name, and io, milltask, halui and the
Python bindings all map that same inode MAP_SHARED. An offline parse run
beside a live session therefore replaces the running machine's tool table
with the compiled-in sample table. O_TRUNC preserves the inode, so nothing
re-maps and nothing is notified: the session simply observes its tools
change. Observed on a machine using [EMCIO]DB_PROGRAM -- 15 tools became the
4 sample entries mid-session, G43 applied 0.0000 for a tool that was no
longer in the table, and the tool-number/drawbar guard inhibited jog and
feed. DB_PROGRAM neither prevents nor repairs it: ioControl.cc creates the
mmap before the DB_ACTIVE branch, and io does not re-read afterwards.
Give sai its own file and unlink it on exit. tool_mmap_close() already
unlinks and tool_mmap_fname() already honours a preset filename -- this only
adds the setter to reach it. io and milltask are untouched and remain the
only creators of the shared file.
Two points from review, both addressed here:
mkstemp(), not a name built from the pid (grandixximo). TMPDIR is
world-writable and a pid is guessable, and the creator opens without O_EXCL
or O_NOFOLLOW, so a predictable name can be pre-created as a symlink and the
victim's rs274 then truncates the attacker's chosen file -- and an attacker
can blanket a pid range in advance. mkstemp() creates it atomically with
O_EXCL and mode 0600, and TOOL_MMAP_CREATOR_OPEN_FLAGS gains O_NOFOLLOW so
the creator refuses a symlink at that path even if one appears in the gap.
tool_mmap_close() is now safe as an atexit handler (BsAtHome). It called
exit(EXIT_FAILURE) when munmap failed, and calling exit() from within an
atexit handler is undefined behaviour; _exit would skip the remaining
handlers, so that is not the answer either. It now reports the failure,
closes the fd and returns.
Reproduce before the change:
rm -rf /tmp/rsx && mkdir -p /tmp/rsx
HOME=/tmp/rsx rs274 -g /dev/null
# /tmp/rsx/.tool.mmap, last_index=4, holding
# T1 z0.511 d0.125 / T2 z0.100 d0.0625 / T3 z1.273 d0.201 / T99999 P123
After: rs274 writes $TMPDIR/rs274.tool.mmap.XXXXXX, removes it on exit, and
does not open $HOME/.tool.mmap. Verified on a live machine: an rs274 run with
the real $HOME left last_index=15 and every tool untouched.
tzuohann
force-pushed
the
rs274-private-toolmmap
branch
from
August 17, 2026 02:31
74522c6 to
8c8e048
Compare
BsAtHome
reviewed
Aug 17, 2026
BsAtHome
reviewed
Aug 18, 2026
Comment on lines
+240
to
+241
| // close, returns at the tool_mmap_base guard above. | ||
| tool_mmap_base = (char*)0; |
Contributor
There was a problem hiding this comment.
Please use NULL or nullptr instead of manual casts of the value zero (0).
Contributor
Author
There was a problem hiding this comment.
Done with nullptr at 241 and also at 200 (another one).
Contributor
|
BTW, I see this is targeted at 2.9 branch. The same problem in master, I guess? |
BsAtHome, review 2026-08-18: `(char*)0` is a hand-written null pointer. Both occurrences in tooldata_mmap.cc now use nullptr. No behaviour change.
Contributor
Author
Yes. Its in master. I wasn't sure if you want to retarget this, or do a new PR. Just LMK, or DIY. |
Contributor
|
Yes, then you need to retarget this PR to master. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Running
rs274while a LinuxCNC session is up replaces that session's tooltable with the sample one (shipped with lcnc), until restart.
A lucky user will find that conflicting MDI or gcode stops running. A less
lucky user will find code running with offsets from sample tools - potentially
disastrous.
tool_mmap_creator()opens$HOME/.tool.mmapO_RDWR|O_CREAT|O_TRUNC(tooldata_mmap.cc:33, :135) and sai calls it at driver.cc:570 — before
getopt()at :578, so it happens on every invocation,--helpincluded, and-t(read at :583) cannot prevent it.io, milltask, halui and the Python bindings all map that same inode
MAP_SHARED.O_TRUNCkeeps the inode, so nothing re-maps and nothingerrors — the running session simply starts using the sample table.
Reproduce:
Patch:
Seen on 2.9.8, identical at 2.9.10.