sai: give rs274 its own tool mmap instead of truncating $HOME/.tool.mmap - #4385
sai: give rs274 its own tool mmap instead of truncating $HOME/.tool.mmap#4385tzuohann wants to merge 1 commit into
Conversation
3c37f31 to
74522c6
Compare
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.
74522c6 to
8c8e048
Compare
| if (mmap_fd >= 0) { | ||
| close(mmap_fd); | ||
| mmap_fd = -1; | ||
| } | ||
| // Idempotent from here: a second call, or an atexit after an explicit | ||
| // close, returns at the tool_mmap_base guard above. | ||
| tool_mmap_base = (char*)0; | ||
| mmap_is_creator = false; | ||
| } //tool_mmap_close() |
There was a problem hiding this comment.
I should have checked this better when I made my other comment. But, now you also removed the close at the bottom of the function. That means you now will leak the descriptor.
The original code would leak a file on the filesystem, now it is the file descriptor. It can be fixed much easier by making it look like:
void tool_mmap_close()
{
if (!tool_mmap_base) { return; }
// flush mmapped file to filesystem
if (msync(tool_mmap_base, TOOL_MMAP_SIZE, MS_SYNC) == -1) {
perror("tool_mmap_close(): msync fail");
}
if (munmap(tool_mmap_base, TOOL_MMAP_SIZE) < 0) {
perror("tool_mmap_close(): munmapfail");
}
if( unlink(tool_mmap_fname() )) {
perror("tool_mmap_close(): unlink fail");
}
close(creator_fd);
} //tool_mmap_close()There was a problem hiding this comment.
The close is still there, just not on creator_fd:
if (mmap_fd >= 0) {
close(mmap_fd);
mmap_fd = -1;
}
mmap_fd is set by both the creator and user paths after a successful mmap; both close exactly one descriptor. creator_fd is set by creator; user process leaves it 0, so close(creator_fd) there closes stdin. Same reason unlink is guarded by mmap_is_creator.
Full disclosure if not obvious, I have no training with software architecture. Have a background in scientific programming, but that's completely different stuff. So, if I don't understand basic things, just take it as a report instead of a pull request. That may make things go faster. I use claude for all this stuff, but I don't have enough time to properly learn software, not at 42. Just relaying what claude said after quizzing it repeatedly about making sure your comments are addressed.
There was a problem hiding this comment.
No problem, most are not programmers by profession here. We're glad to have people participate and contribute!
Let me have a wider look at this code. Maybe I am barking up the wrong tree. However, there are many oddities with the tool database code, so a closer look may be warranted.
There was a problem hiding this comment.
cool lmk if this works
3 participants
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.