On Sat, Oct 19 2019, [email protected] wrote: > On Sat, 19 Oct 2019 16:32:56 +0100, Jeremie Courreges-Anglas > <[email protected]> said: >> > Before: >> > $ HISTSIZE=1000000000 /bin/sh >> > /bin/sh: internal error: unable to allocate memory >> > <crashes> <--- no ksh >> Could you provide more details please? A fatal error and a crash are >> very different problems. > > Yes, it's a fatal error resulting in a termination since nothing > "catches" the internal_errorf() from the allocation failure. > >> Does "no ksh" imply that the *current* shell >> exited or crashed, leaving you with no shell at all? > > It does not crash the current shell. But if the current shell is not > ksh, then the experience is: > > bash$ /bin/sh > ksh: internal error: unable to allocate memory > bash$ > > Which is not hugely informative. My actual experience was that running > "some commands" failed with this error, because they ran shell scripts > either directly or indirectly. So I was not directly running ksh or > /bin/sh when I first got these errors. > > I don't have a specific use case where this would lock you out of > troubleshooting, but seems plausible. > >> Note that setting HISTSIZE like this affects your *current* shell and >> prevents it to run /bin/sh. Use env to only affect the child shell: > > My current shell is usually bash, which handles large HISTSIZE (I > don't know if by mmaping the file, a capped effective size, or some > other way).
Ok, thankd for those details. >> Your diff doesn't apply as is. > > Sorry about that. I used github.com/openbsd/src HEAD, and am looking > forward to got (https://gameoftrees.org/). Maybe I still screwed up somehow. > >> After applying it, the child ksh will >> indeed keep running even if setting HISTSIZE fails. But it reliably >> crashes on exit if HISTSIZE was set in its startup environment: >> >> --8<-- >> ritchie /usr/src/bin/ksh$ env ENV=/dev/null HISTFILE=ksh_history >> HISTSIZE=1000 obj/ksh >> ritchie$ ^D >> Unable to set the history size to 0 >> Trace/BPT trap (core dumped) >> -->8-- > > Confirmed that I made it crash on exit if and only if the HISTSIZE is > *low*. I was missing a quitenv(NULL) as the last line in > sethistsize(). After adding that it works as it should for both normal > and oversized HISTSIZE. > > I'm not familiar with the "environment" concept in the code, and could > not find it documented, thus missed that. > >> I think we can do ourselves a favor and not involve unwinding/setjmps >> for this simple error case. >> Using alloc.c to allocate histbase is not useful, > > That would be ideal, yes, if there's no reason to use alloc.c here. > >> so just switch to reallocarray. Allocation failure at startup >> is still fatal because the code can't cope with a NULL history array. >> >> ok? > > Yeah, that looks better, and I can confirm that it works as expected > for me now, both for small and large HISTSIZE. Committed. > I don't know what the general philosophy of footguns are for you, but > a cap on HISTSIZE could prevent someone having ksh consume 764MB RAM > (as seen by top with HISTSIZE=100000000). The allocation policy could be smarter indeed. Pre-allocating such a huge array doesn't bring much value. If someone wants to take a stab at it they're welcome to do so. > Thanks! Sure. Thank you too for caring. >> >> >> Index: history.c >> =================================================================== >> RCS file: /cvs/src/bin/ksh/history.c,v >> retrieving revision 1.82 >> diff -u -p -r1.82 history.c >> --- history.c 28 Jun 2019 13:34:59 -0000 1.82 >> +++ history.c 19 Oct 2019 15:29:04 -0000 >> @@ -554,6 +554,7 @@ void >> sethistsize(int n) >> { >> if (n > 0 && (uint32_t)n != histsize) { >> + char **tmp; > > why not void* ? Because char ** is the right type here. I see no reason to use the weaker semantics of void * where they aren't not needed. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
