From b07a850d0f27677b5e235cfe36d8b4f8946e3af6 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Mon, 24 Feb 2025 08:35:12 +0000 Subject: [PATCH 1/7] configure.in: getrlimit setrlimit sys/resource.h --- scripts/configure.in | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/scripts/configure.in b/scripts/configure.in index 4f76cb730..c0be24f99 100644 --- a/scripts/configure.in +++ b/scripts/configure.in @@ -132,6 +132,12 @@ AC_HEADER_STDC dnl Need either setenv or putenv AC_CHECK_FUNCS(setenv putenv) +dnl Check for getrlimit +AC_CHECK_FUNCS(getrlimit) + +dnl Check for setrlimit +AC_CHECK_FUNCS(setrlimit) + dnl Check for vfork AC_CHECK_FUNC(vfork) @@ -150,6 +156,9 @@ AC_CHECK_HEADERS(param.h) dnl Check for AC_CHECK_HEADERS(paths.h) +dnl Check for +AC_CHECK_HEADERS(sys/resource.h) + dnl Check for AC_CHECK_HEADERS(sys/time.h) From c50240b0367ee098db60d3c260ea9bccf76a6bb4 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Mon, 24 Feb 2025 08:37:37 +0000 Subject: [PATCH 2/7] configure: autoconf regen (2.69) getrlimit setrlimit sys/resource.h --- scripts/configure | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/scripts/configure b/scripts/configure index 53e76d25d..51ca793d9 100755 --- a/scripts/configure +++ b/scripts/configure @@ -4893,6 +4893,30 @@ fi done +for ac_func in getrlimit +do : + ac_fn_c_check_func "$LINENO" "getrlimit" "ac_cv_func_getrlimit" +if test "x$ac_cv_func_getrlimit" = xyes; then : + cat >>confdefs.h <<_ACEOF +#define HAVE_GETRLIMIT 1 +_ACEOF + +fi +done + + +for ac_func in setrlimit +do : + ac_fn_c_check_func "$LINENO" "setrlimit" "ac_cv_func_setrlimit" +if test "x$ac_cv_func_setrlimit" = xyes; then : + cat >>confdefs.h <<_ACEOF +#define HAVE_SETRLIMIT 1 +_ACEOF + +fi +done + + ac_fn_c_check_func "$LINENO" "vfork" "ac_cv_func_vfork" if test "x$ac_cv_func_vfork" = xyes; then : @@ -4964,6 +4988,19 @@ fi done +for ac_header in sys/resource.h +do : + ac_fn_c_check_header_mongrel "$LINENO" "sys/resource.h" "ac_cv_header_sys_resource_h" "$ac_includes_default" +if test "x$ac_cv_header_sys_resource_h" = xyes; then : + cat >>confdefs.h <<_ACEOF +#define HAVE_SYS_RESOURCE_H 1 +_ACEOF + +fi + +done + + for ac_header in sys/time.h do : ac_fn_c_check_header_mongrel "$LINENO" "sys/time.h" "ac_cv_header_sys_time_h" "$ac_includes_default" From 7bb5777a036097042b0ff76c12e1e6e4e180c107 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Mon, 24 Feb 2025 08:39:27 +0000 Subject: [PATCH 3/7] tclmagic.c: RLIMIT_NOFILE to 4096 on startup (only for TCL9 envs) --- tcltk/tclmagic.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tcltk/tclmagic.c b/tcltk/tclmagic.c index a477bd4ae..a616a4005 100644 --- a/tcltk/tclmagic.c +++ b/tcltk/tclmagic.c @@ -25,6 +25,9 @@ #include #include #include +#ifdef HAVE_SYS_RESOURCE_H +#include +#endif #include "tcltk/tclmagic.h" #include "utils/main.h" @@ -530,6 +533,61 @@ MakeWindowCommand(char *wname, MagWindow *mw) freeMagic(tclcmdstr); } +#ifdef HAVE_SETRLIMIT +static int +process_rlimit_nofile_ensure(rlim_t nofile) +{ + struct rlimit rlim; + int err = getrlimit(RLIMIT_NOFILE, &rlim); + if (err < 0) + return err; + rlim_t rlim_cur = rlim.rlim_cur; + /* nofile != RLIM_INFINITY && rlim.rlim_max != RLIM_INFINITY */ + if (nofile > rlim.rlim_max && nofile != rlim.rlim_max) + return -1; + if (rlim.rlim_cur < nofile || nofile == RLIM_INFINITY) + { + rlim.rlim_cur = nofile; + err = setrlimit(RLIMIT_NOFILE, &rlim); + } + if (err != 0) + TxPrintf("WARNING: process_rlimit_nofile_ensure(%lu) = %d (%d) [rlim_cur=%lu rlim_max=%lu]\n", nofile, err, errno, rlim_cur, rlim.rlim_max); + return err; +} +#endif /* HAVE_SETRLIMIT */ + +/* this function encapsulates the default policy on startup */ +static int +process_rlimit_startup_check(void) +{ +#ifdef HAVE_GETRLIMIT + #if TCL_MAJOR_VERSION < 9 + /* TCL8 has select() support and no support for poll/epoll for the main event loop */ + struct rlimit rlim; + int err = getrlimit(RLIMIT_NOFILE, &rlim); + if (err < 0) + return err; + if (rlim.rlim_cur > FD_SETSIZE) + { + TxPrintf("WARNING: RLIMIT_NOFILE is above %d and Tcl_Version<9 this may cause runtime issues [rlim_cur=%lu]\n", FD_SETSIZE, rlim.rlim_cur); + return -1; + } + return 0; + #else + #ifdef HAVE_SETRLIMIT + /* TCL9 has poll/epoll support for the main event loop, + * ifdef due to rlim_t type availbility + */ + return process_rlimit_nofile_ensure(4096); + #else + return -1; + #endif /* HAVE_SETRLIMIT */ + #endif /* TCL_MAJOR_VERSION < 9 */ +#else + return -1; +#endif /* HAVE_GETRLIMIT */ +} + /*------------------------------------------------------*/ /* Main startup procedure */ /*------------------------------------------------------*/ @@ -588,6 +646,8 @@ _magic_initialize(ClientData clientData, TxPrintf("Using the terminal as the console.\n"); TxFlushOut(); + process_rlimit_startup_check(); + if (mainInitAfterArgs() != 0) goto magicfatal; /* Registration of commands is performed after calling the */ From d7a91c83bb3563990d472c287051bc7778838e8e Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Mon, 24 Feb 2025 08:44:09 +0000 Subject: [PATCH 4/7] grMain.c select() usage fix and protect from higher numbered fd's The old code would only work is the fileno(stream) returned an fd in the range 0 <= 19. It would silently fail, if the fd was in the range 20..1023 because FD_SET() would work and syscall select() would be limited to only look at the first 20 fd's. Ignoring any fd's higher even if set. This would theoretically cause high CPU usage due to select() never blocking because there are no active fd's in the fd_set as far as the kernel interprets the request and the kernel would immediately return. But reading the code the 1st argument to select() seems self limiting for no good reason. It should be fileno(stream)+1 as documented in man select(2). Added the assertion as well, because we are trying to allow magic to use fd's beyond the standard environmental limits. So it becomes an assertion condition if the fd is outside the range 0..1023 because the FD_SET() macro will not operate correctly / undefined-behaviour. I can't find any user of this func in the codebase right now. If you look at sim/SimRsim.c and the use of select() there, it is correctly using select() to wait on a single fd over there. This commit changes this code to match this correct usage. --- graphics/grMain.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/graphics/grMain.c b/graphics/grMain.c index 9d565e3e3..2e4652975 100644 --- a/graphics/grMain.c +++ b/graphics/grMain.c @@ -457,8 +457,15 @@ grFgets(str, n, stream, name) twentySecs.tv_sec = 20; twentySecs.tv_usec = 0; + const int fd = fileno(stream); + ASSERT(fd >= 0 && fd < FD_SETSIZE, "fd>=0&&fd= FD_SETSIZE) + { + TxError("WARNING: grFgets(fd=%d) called with fd out of range 0..%d\n", fd, FD_SETSIZE-1); + return NULL; /* allowing things to continue is UB */ + } FD_ZERO(&fn); - FD_SET(fileno(stream), &fn); + FD_SET(fd, &fn); newstr = str; n--; if (n < 0) return (char *) NULL; @@ -470,13 +477,13 @@ grFgets(str, n, stream, name) int sel; f = fn; - sel = select(TX_MAX_OPEN_FILES, &f, (fd_set *) NULL, (fd_set *) NULL, &threeSec); + sel = select(fd + 1, &f, (fd_set *) NULL, (fd_set *) NULL, &threeSec); if (sel == 0) { TxError("The %s is responding slowly, or not at all.\n", name); TxError("I'll wait for 20 seconds and then give up.\n"); f = fn; - sel = select(TX_MAX_OPEN_FILES, &f, (fd_set *) NULL, + sel = select(fd + 1, &f, (fd_set *) NULL, (fd_set *) NULL, &twentySecs); if (sel == 0) { From bda57415418422ad6903579104d8f68b25811cdc Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Mon, 24 Feb 2025 08:45:23 +0000 Subject: [PATCH 5/7] select() API usages add ASSERT() to validate fd number is in-range This encapsulates the expectation the 'fd' is in the permitted range for the standard sizes 'fd_set'. This is so there is some form of detection with issues in this area, if the RLIMIT_NOFILE limit is increased. --- sim/SimRsim.c | 7 +++++++ textio/txCommands.c | 14 +++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/sim/SimRsim.c b/sim/SimRsim.c index 5088e8c33..9acbe75d7 100644 --- a/sim/SimRsim.c +++ b/sim/SimRsim.c @@ -650,6 +650,13 @@ SimFillBuffer(buffHead, pLastChar, charCount) FD_ZERO(&exceptfds); #endif /* SYSV */ + ASSERT(pipeIn >= 0 && pipeIn < FD_SETSIZE, "pipeIn>=0&&pipeIn= FD_SETSIZE) + { + TxError("WARNING: SimFillBuffer(fd=%d) called with fd out of range 0..%d\n", pipeIn, FD_SETSIZE-1); + return -1; /* allowing things to continue is UB */ + } + nfd = pipeIn + 1; try_again: diff --git a/textio/txCommands.c b/textio/txCommands.c index b002f04e2..e313f179f 100644 --- a/textio/txCommands.c +++ b/textio/txCommands.c @@ -489,6 +489,12 @@ TxAdd1InputDevice( ClientData cdata) { fd_set fs; + ASSERT(fd >= 0 && fd < FD_SETSIZE, "fd>=0&&fd= FD_SETSIZE) + { + TxError("WARNING: TxAdd1InputDevice(fd=%d) called with fd out of range 0..%d\n", fd, FD_SETSIZE-1); + return; /* allowing things to continue is UB */ + } FD_ZERO(&fs); FD_SET(fd, &fs); TxAddInputDevice(&fs, inputProc, cdata); @@ -524,8 +530,14 @@ void TxDelete1InputDevice( int fd) { - int i, j; + ASSERT(fd >= 0 && fd < FD_SETSIZE, "fd>=0&&fd= FD_SETSIZE) + { + TxError("WARNING: TxDelete1InputDevice(fd=%d) called with fd out of range 0..%d\n", fd, FD_SETSIZE-1); + return; /* allowing things to continue is UB */ + } + int i, j; for (i = 0; i <= txLastInputEntry; i++) { FD_CLR(fd, &(txInputDevice[i].tx_fdmask)); From 95f50316311ff0c55ab1810743ca895370e99c99 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Mon, 24 Feb 2025 08:49:07 +0000 Subject: [PATCH 6/7] txCommands.c txInputDevRec FD select() usage overhaul There are numerous concerns with the original code from a read through. The #define TX_MAX_OPEN_FILES 20 is badly named, the txCommand.c uses fd_set to hold active FD for each device, and makes no attempt to bounds check the 'fd' or 'fd_set' of any TxAdd1InputDevice() is in range. The real name of this variable should be TX_MAX_INPUT_DEVICES as it related to the fixed compile time slots available for devices, each input device must have at least one active FD and it can be in the range that fd_set allows, which is 0..1023 on linux. So based on this being the original intention, due to the way the code is constructed and API calls made available, the file has been reworked to allow all these considerations at the same time. Now the design should be FD clean for FDs in the range 0..1023 instead of being artificially clamped to the first 20 FDs being valid input devices. Renaming of variable 'i' to 'fd' when it relates to an fd number, so all uses of FD_XXXX() should use fd, this helps clarify the different domain the number relates to. When 'i' is used it relates to the index into the txInputDevRec array. A large part of the concerns relate to trying to mix the two numbering domains interchangeably, just using a different name really clears up understanding to the reader. Other items that look like errors TxDelete1InputDevice() will shuffle-remove entries with no active FD, but it is iterating an array it is also modifying, so it looks like it would have incorrectly skipped the next item that should be inspected just after a shuffle-removal occurred. The various iterators that works from 0..TX_MAX_OPEN_FILES incorrectly limited the visibility of the routines to the first 20 FDs instead of the full FD_SETSIZE the TxAddInputDevice API call allows to be registered. Passing in TxAdd1InputDevice with an fd above 19 would have resulted in everything looking successful until select() was used, then the kernel would likely not sleep/wait because the input fd_set would look blank due to being clipped by nfds=TX_MAX_OPEN_FILES (instead of that plus 1). The use of TX_MAX_OPEN_FILES in select instead of TX_MAX_OPEN_FILES+1 for the 'nfds' field would have meant a device with fd=19 would not work as the design intended. The define definition has been removed from the module public header, I assume it was there for use by another module, but the incorrect select() usage has been corrected over there in a previous commit. So now the define can be maintained near the array it relates to. While the code might looks less 'efficient' as it is now sweeping all 1024 FDs when input devices during add/remove (hopefully there maybe some compiler auto-vectorization/tzcnt use there). The main event loop is slightly more 'efficient' (especially for the single device with input fd=0 case) as it will only check the 1 FD each input event loop iteration, not check all 20 FDs for data readyness everytime. --- textio/textio.h | 2 - textio/txCommands.c | 97 +++++++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 28 deletions(-) diff --git a/textio/textio.h b/textio/textio.h index c749ea7e7..2dd500466 100644 --- a/textio/textio.h +++ b/textio/textio.h @@ -110,6 +110,4 @@ extern void TxInit(void); extern void TxInitReadline(void); #endif -#define TX_MAX_OPEN_FILES 20 - #endif /* _TEXTIO_H */ diff --git a/textio/txCommands.c b/textio/txCommands.c index e313f179f..cc71bc352 100644 --- a/textio/txCommands.c +++ b/textio/txCommands.c @@ -64,11 +64,17 @@ bool TxDebug = FALSE; /* A mask of the file descriptors for all input devices. */ static fd_set txInputDescriptors; +/* Used for the 'nfds' field for select() syscall, the highest + * fd number that is set in the txInputDescriptors bitmask. + * Don't forget to plus 1 for select(). + */ +static int txInputDescriptors_nfds; +#define TX_MAX_INPUT_DEVICES 20 /* A table of all input devices. */ -static txInputDevRec txInputDevice[TX_MAX_OPEN_FILES]; -static int txLastInputEntry = -1; +static txInputDevRec txInputDevice[TX_MAX_INPUT_DEVICES]; +static int txLastInputEntry; /* The current point -- reset by the 'setpoint' command and for each * interactive command. Calls to TxClearPoint clear previous setpoints, @@ -130,6 +136,9 @@ static TxCommand *lisp_cur_cmd = NULL; * * FD_IsZero -- * FD_OrSet -- + * FD_MaxFd -- + * Returns the highest 'fd' in the mask for select(2) nfds argument, or + * -1 when 'fdmask' is empty. * * Routines for manipulating set of file descriptors. * @@ -140,9 +149,9 @@ bool FD_IsZero( const fd_set *fdmask) { - int i; - for (i = 0; i <= TX_MAX_OPEN_FILES; i++) - if (FD_ISSET(i, fdmask)) return FALSE; + int fd; + for (fd = 0; fd < FD_SETSIZE; fd++) + if (FD_ISSET(fd, fdmask)) return FALSE; return TRUE; } @@ -151,9 +160,30 @@ FD_OrSet( const fd_set *fdmask, fd_set *dst) { - int i; - for (i = 0; i <= TX_MAX_OPEN_FILES; i++) - if (FD_ISSET(i, fdmask)) FD_SET(i, dst); + int fd; + for (fd = 0; fd < FD_SETSIZE; fd++) + if (FD_ISSET(fd, fdmask)) FD_SET(fd, dst); +} + +/* A bitmask find max bit set operation */ +int +FD_MaxFd(fdmask) + const fd_set *fdmask; +{ + int fd; + for (fd = FD_SETSIZE-1; fd >= 0; fd--) /* backwards */ + if (FD_ISSET(fd, fdmask)) return fd; + return -1; +} + +/* update txInputDescriptors_nfds with the correct value + * call this everytime txInputDescriptors is modified + */ +static void +TxInputDescriptorsRecalc(void) +{ + int nfds = FD_MaxFd(&txInputDescriptors); + txInputDescriptors_nfds = (nfds >= 0) ? nfds : 0; } /* @@ -468,18 +498,20 @@ TxAddInputDevice( * it is called. */ { - int i; TxDeleteInputDevice(fdmask); - if (txLastInputEntry + 1 == TX_MAX_OPEN_FILES) + if (txLastInputEntry == TX_MAX_INPUT_DEVICES) { TxError("Too many input devices.\n"); return; } - txLastInputEntry++; + txInputDevice[txLastInputEntry].tx_fdmask = *fdmask; txInputDevice[txLastInputEntry].tx_inputProc = inputProc; txInputDevice[txLastInputEntry].tx_cdata = cdata; + txLastInputEntry++; + FD_OrSet(fdmask, &txInputDescriptors); + TxInputDescriptorsRecalc(); } void @@ -520,10 +552,9 @@ TxDeleteInputDevice( * no longer active. */ { - int i; - - for (i = 0; i <= TX_MAX_OPEN_FILES; i++) - if (FD_ISSET(i, fdmask)) TxDelete1InputDevice(i); + int fd; + for (fd = 0; fd < FD_SETSIZE; fd++) + if (FD_ISSET(fd, fdmask)) TxDelete1InputDevice(fd); } void @@ -537,18 +568,30 @@ TxDelete1InputDevice( return; /* allowing things to continue is UB */ } - int i, j; - for (i = 0; i <= txLastInputEntry; i++) +restart: { - FD_CLR(fd, &(txInputDevice[i].tx_fdmask)); - if (FD_IsZero(&txInputDevice[i].tx_fdmask)) + int i; + for (i = 0; i < txLastInputEntry; i++) { - for (j = i+1; j <= txLastInputEntry; j++) - txInputDevice[j-1] = txInputDevice[j]; - txLastInputEntry--; + FD_CLR(fd, &txInputDevice[i].tx_fdmask); + if (FD_IsZero(&txInputDevice[i].tx_fdmask)) + { + int j; + for (j = i+1; j < txLastInputEntry; j++) + txInputDevice[j-1] = txInputDevice[j]; + txLastInputEntry--; + /* we shuffled entries down one, so txInputDevice[i] now + * looks at potential next one to inspect, but we're about + * to i++ (if we continue) and that will incorrectly skip + * inspection of it. + * lets just start over + */ + goto restart; + } } } FD_CLR(fd, &txInputDescriptors); + TxInputDescriptorsRecalc(); } @@ -968,7 +1011,7 @@ TxGetInputEvent( { if (returnOnSigWinch && SigGotSigWinch) return gotSome; inputs = txInputDescriptors; - numReady = select(TX_MAX_OPEN_FILES, &inputs, (fd_set *)NULL, + numReady = select(txInputDescriptors_nfds + 1, &inputs, (fd_set *)NULL, (fd_set *)NULL, waitTime); if (numReady <= 0) FD_ZERO(&inputs); /* no fd is ready */ } while ((numReady <= 0) && (errno == EINTR)); @@ -978,20 +1021,21 @@ TxGetInputEvent( perror("magic"); } - for (i = 0; i <= txLastInputEntry; i++) + for (i = 0; numReady && i < txLastInputEntry; i++) { /* This device has data on its file descriptor, call * it so that it can add events to the input queue. */ - for (fd = 0; fd < TX_MAX_OPEN_FILES; fd++) { + for (fd = 0; fd <= txInputDescriptors_nfds; fd++) { if (FD_ISSET(fd, &inputs) && - FD_ISSET(fd, &(txInputDevice[i].tx_fdmask))) { + FD_ISSET(fd, &txInputDevice[i].tx_fdmask)) { lastNum = txNumInputEvents; (*(txInputDevice[i].tx_inputProc)) (fd, txInputDevice[i].tx_cdata); /** @invoke cb_textio_input_t */ FD_CLR(fd, &inputs); /* Did this driver choose to add an event? */ if (txNumInputEvents != lastNum) gotSome = TRUE; + numReady--; } } } @@ -1711,6 +1755,7 @@ txCommandsInit(void) txZeroTime.tv_sec = 0; txZeroTime.tv_usec = 0; FD_ZERO(&txInputDescriptors); + txInputDescriptors_nfds = 0; DQInit(&txInputEvents, 4); DQInit(&txFreeEvents, 4); DQInit(&txFreeCommands, 4); From a742413749c84f407c73ff101ae68d9698f45907 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Mon, 24 Feb 2025 15:24:59 +0000 Subject: [PATCH 7/7] txCommands.c TxGetInputEvent() rework main loop to iterate less Original version would iterate exhaustively, even when it was not necessary. This version seeks to do the minimum amount of iteration work based on the information available. --- textio/txCommands.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/textio/txCommands.c b/textio/txCommands.c index cc71bc352..5e05a5f33 100644 --- a/textio/txCommands.c +++ b/textio/txCommands.c @@ -1021,22 +1021,33 @@ TxGetInputEvent( perror("magic"); } - for (i = 0; numReady && i < txLastInputEntry; i++) + /* 0..1023 using numReady==0 to terminate early */ + for (fd = 0; numReady && fd <= txInputDescriptors_nfds; fd++) { - /* This device has data on its file descriptor, call - * it so that it can add events to the input queue. - */ - for (fd = 0; fd <= txInputDescriptors_nfds; fd++) { - if (FD_ISSET(fd, &inputs) && - FD_ISSET(fd, &txInputDevice[i].tx_fdmask)) { - lastNum = txNumInputEvents; - (*(txInputDevice[i].tx_inputProc)) - (fd, txInputDevice[i].tx_cdata); /** @invoke cb_textio_input_t */ - FD_CLR(fd, &inputs); - /* Did this driver choose to add an event? */ - if (txNumInputEvents != lastNum) gotSome = TRUE; - numReady--; - } + if (!FD_ISSET(fd, &inputs)) + continue; /* this fd is was not monitored or is not ready */ + + /* find the input device receiver entry */ + for (i = 0; i < txLastInputEntry; i++) + { + if (!FD_ISSET(fd, &txInputDevice[i].tx_fdmask)) + continue; + + /* This device has data on its file descriptor, call + * it so that it can add events to the input queue. + */ + lastNum = txNumInputEvents; + + (*txInputDevice[i].tx_inputProc)(fd, txInputDevice[i].tx_cdata); + + /* Did this driver choose to add an event? */ + if (txNumInputEvents != lastNum) gotSome = TRUE; + numReady--; + + /* original code would FD_CLR() which would effectively break here + * so this only allows the first found receiver to receive the data + */ + break; } } /*