On Thu, Aug 24 2017, Stuart Henderson <[email protected]> wrote:
> On 2017/08/24 17:42, Jeremie Courreges-Anglas wrote:
>> On Wed, Aug 23 2017, Stuart Henderson <[email protected]> wrote:
>> > On 2017/08/15 11:57, Jeremie Courreges-Anglas wrote:
>> >> CVSROOT: /cvs
>> >> Module name: src
>> >> Changes by: [email protected] 2017/08/15 11:57:57
>> >>
>> >> Modified files:
>> >> bin/ksh : alloc.c
>> >>
>> >> Log message:
>> >> Remove expensive pointer check in afree()
>> >>
>> >> The check added in rev 1.8 walks the whole freelist to catch cases where
>> >> an unknown pointer is passed to afree(); but it can't catch cases
>> >> whether the struct link has been corrupted by an invalid memory write.
>> >> And it becomes very expensive when you have lots of items in an area
>> >> (for example with a huge HISTSIZE).
>> >>
>> >> Discussed with & ok millert@ tb@
>> >>
>> >
>> > I'm not certain that it's a result of this change, but I updated to a
>> > snapshot past this change yesterday and today in one of my open shells
>> > I started getting "ksh: history file is corrupt" showing up when trying
>> > to run any commands (the command is then not executed). Opening a new
>> > terminal I get a hang (interruptable with ^C):
>> >
>> > load: 0.38 cmd: ksh 2244 [lockf] 0.02u 0.02s 0% 243k
>> >
>> > My histfile is currently 1197 lines:
>> >
>> > $ wc .ksh_history
>> > 1197 12866 274756 .ksh_history
>> >
>> > Some of them are fairly long lines (e.g. 4096, 8465, 8482, 8345 chars)
>> > and I probably haven't had quite such long lines before since the switch
>> > to the plain history file, so if it's connected with that, it could also
>> > have been a problem prior to the recent change and I just didn't run into
>> > it before.
>>
>> An overlong (>= 4096) line will result in the "ksh: history file is
>> corrupt" warning, cvs says that this behavior was introduced with the
>> current plaintext history file support. But it shouldn't result in hung
>> interactive shell processes. Could you please track this down to
>> a short histfile that you could share?
>
> I've been trying to re-trigger the hang this evening, I got it once
> with the untrimmed history file that I'd backed up, but haven't repeated
> it with any trimmed one, or the untrimmed one again now. Given the wchan
> "lockf" I think this must have been in the history_lock() loop? Perhaps
> there was a broken ksh process holding onto the file in a terminal that
> I've since closed..
That would explain, but it's not clear to me how to trigger this.
> These long command lines aren't *all* that unusual if you've pasted
> a cc/linker command into fc and are fiddling with parameters trying to
> figure out how to build a recalcitrant port :)
Supporting arbitrarily long lines sounds better than a hardcoded 4096,
but doing it cleanly doesn't look trivial. So here's a diff to ignore
history lines that are too long. This should help people who had very
long lines in their old binary histfile, which afaik didn't had length
limitations.
There is no special cleanup done for those lines, they will just
disappear when the shell rewrites the history file, ie when history
lines count reaches 1.25 * HISTSIZE. The warning ensures that the user
knows about those ignored lines and happens only once in the shell life
cycle.
Also I'm adding a warning for cases where we hit an error when reading
the history file. Being silent here doesn't help the user much.
Ramdisks build tested in an mkr. ok?
Index: history.c
===================================================================
RCS file: /d/cvs/src/bin/ksh/history.c,v
retrieving revision 1.70
diff -u -p -r1.70 history.c
--- history.c 31 Aug 2017 11:10:03 -0000 1.70
+++ history.c 3 Sep 2017 04:05:09 -0000
@@ -29,7 +29,7 @@
static void history_write(void);
static FILE *history_open(void);
-static int history_load(Source *);
+static void history_load(Source *);
static void history_close(void);
static int hist_execute(char *);
@@ -728,7 +728,39 @@ history_close(void)
}
}
-static int
+static char *
+history_getline(char *str, int size)
+{
+ char *newline;
+
+ do {
+ str = fgets(str, size, histfh);
+ if (str == NULL)
+ break;
+ if ((newline = strchr(str, '\n')) == NULL) {
+ static int toolong = 0;
+ if (!toolong) {
+ toolong = 1;
+ warningf(0, "line(s) longer than %d "
+ "bytes in history file (ignored)",
+ size - 1);
+ }
+ do
+ str = fgets(str, size, histfh);
+ while (str != NULL && strchr(str, '\n') == NULL);
+ } else
+ *newline = '\0';
+ } while (str != NULL && newline == NULL);
+
+ if (str == NULL && ferror(histfh)) {
+ warningf(0, "error while reading history file");
+ clearerr(histfh);
+ }
+
+ return str;
+}
+
+static void
history_load(Source *s)
{
char *p, encoded[LINE + 1], line[LINE + 1];
@@ -737,14 +769,9 @@ history_load(Source *s)
/* just read it all; will auto resize history upon next command */
for (line_co = 1; ; line_co++) {
- p = fgets(encoded, sizeof(encoded), histfh);
- if (p == NULL || feof(histfh) || ferror(histfh))
+ p = history_getline(encoded, sizeof(encoded));
+ if (p == NULL)
break;
- if ((p = strchr(encoded, '\n')) == NULL) {
- bi_errorf("history file is corrupt");
- return 1;
- }
- *p = '\0';
s->line = line_co;
s->cmd_offset = line_co;
strunvis(line, encoded);
@@ -752,8 +779,6 @@ history_load(Source *s)
}
history_write();
-
- return 0;
}
#define HMAGIC1 0xab
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
