On Sat, 19 Oct 2019 16:32:56 +0100, Jeremie Courreges-Anglas <j...@wxcvbn.org> 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). > 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. 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). Thanks! > > > 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* ? > int offset = histptr - history; > > /* save most recent history */ > @@ -566,10 +567,15 @@ sethistsize(int n) > memmove(history, histptr - offset, n * sizeof(char *)); > } > > - histsize = n; > - histbase = areallocarray(histbase, n + 1, sizeof(char *), > APERM); > - history = histbase + 1; > - histptr = history + offset; > + tmp = reallocarray(histbase, n + 1, sizeof(char *)); > + if (tmp != NULL) { > + histbase = tmp; > + histsize = n; > + history = histbase + 1; > + histptr = history + offset; > + } else > + warningf(false, "resizing history storage: %s", > + strerror(errno)); > } > } > > @@ -613,8 +619,10 @@ init_histvec(void) > * allocate one extra element so that histptr always > * lies within array bounds > */ > - histbase = areallocarray(NULL, histsize + 1, sizeof(char *), > - APERM); > + histbase = reallocarray(NULL, histsize + 1, sizeof(char *)); > + if (histbase == NULL) > + internal_errorf("allocating history storage: %s", > + strerror(errno)); > *histbase = NULL; > history = histbase + 1; > histptr = history - 1; > -- typedef struct me_s { char name[] = { "Thomas Habets" }; char email[] = { "tho...@habets.se" }; char kernel[] = { "Linux" }; char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt"; }; char pgp[] = { "9907 8698 8A24 F52F 1C2E 87F6 39A4 9EEA 460A 0169" }; char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" }; } me_t;
