Bk changes <path> stops when it hits a (maximally?) long comment

Bugs
1

In (our-ref) D131514, one of our users reported that bk changes was only displaying the most recent month or so’s changesets in our “misc” repository, unlike cd && bk changes. I see from the bk source that passing on the command line makes bk use a bkd, even if it’s a local path. I couldn’t reproduce the issue on bk’s own repository until I accidentally created a long dspec. Watch it fail with a line 1022 characters long:

martind@swiftboat:~/download/bk/dev$ bk changes -q -n -r+ -d"$(ruby -we 'puts("." * 1021)')" | wc -l
1
martind@swiftboat:~/download/bk/dev$ bk changes -q -n -r+ -d"$(ruby -we 'puts("." * 1022)')" | wc -l
1
martind@swiftboat:~/download/bk/dev$ bk changes -q -n -r+ -d"$(ruby -we 'puts("." * 1021)')" . | wc -l
1
martind@swiftboat:~/download/bk/dev$ bk changes -q -n -r+ -d"$(ruby -we 'puts("." * 1022)')" . | wc -l
0
martind@swiftboat:~/download/bk/dev$

Comments in most of our repositories seem to have been involuntarily wrapped, I assume by some bk command, as we’re clearly in full infinite monkey ^W^W Tolstoy mode, at 1020 characters (bk changes indents them with two spaces by default):

martind@swiftboat:~/work/cannon3$ bk changes | wc -L
1022
martind@swiftboat:~/work/cannon3$

martind@swiftboat:~/work/misc$ bk changes | wc -L
1022
martind@swiftboat:~/work/misc$

We do, though, have at least one line in a check-in comment that’s getting on for a KiB and a half:

martind@swiftboat:~/work/cornet3$ bk changes | wc -L
1363
martind@swiftboat:~/work/cornet3$

This seems to fix my above test cases:

--- 1.207/src/changes.c	2016-09-30 07:58:23 -07:00
+++ edited/src/changes.c	2017-08-22 17:06:02 -07:00
@@ -1720,7 +1720,7 @@ changes_part1
 changes_part1(remote *r, char **av, char *key_list)
 {
 	int	flags, fd, rc, rcsets = 0, rtags = 0;
-	char	buf[MAXPATH];
+	char	buf[MAXLINE];
 
 	if (bkd_connect(r, 0)) return (-1);
 	if (send_part1_msg(r, av)) return (-1);

That clearly does nothing about the distressing lack of error reporting, should the new limit prove to be inadequate too, but perhaps that’s a non-issue.

2

Looks like a classic Larry buffer overrun. I’ll take a look and see if I can fix it.

3

Your diffs didn’t come through on this forum, I’m guessing you changed the bug to MAXLINE which is 2048.
Which is just kicking the can down the road. That said, given the memory we have these days I’d be inclined to kick MAXLINE up to 4096 and kick the can farther down the road. I can make that change and run regressions and see if it wacks performance at all, I suspect it won’t.

The bigger question is can we figure that we got a dspec that is bigger than MAXLINE? And sure we can, that’s an easy test up top. That said, I’d like to have a day where I’m not busy with tractor stuff (my retirement job is repairing tractors and doing tractor work) where I could audit the code and try and walk all the paths and make sure that all the paths are using MAXLINE for dspec args. I’m 100% sure that Wayne would be better at that than me, did you guys ever work out a contract? If not, I’ll take a look but as I said over on hacker news, I used to be able to switch on the programmer zone, these days it just comes to me. Rarely. It does happen but not like when I was 25 or 30.

4

I’ve thrown some markup around the patch but, yes, buf needed to be bigger. MAXLINE seemed a better fit than MAXPATH and there was nearby precedent. If check-in comment lines are being hard-wrapped at 1022 bytes, then I might hope it’d kick the can another twenty years down the road. If only there weren’t that existence proof for at least one longer check-in comment line. I can look into where that came from, although it’s doubtful I’ll be able to find much more than which version we were probably using at the time.

I heard that our bureaucracy finally approved Wayne on Monday, so I was hoping we’d soon be able to get back to matters technical. The hope had occurred that he might chime in here and bill us for his time. I’m in no rush on this, not least as, looking at the bk history of bk itself, it looked to me like the bug had been there since day zero of bk changes in 2001. I’ll spare the blushes but it wasn’t committed as lm.

5

I fixed the quoting on Martin’s original post so the same command lines work. Yes, I fixed changes itself to support arbitrarily sized dspecs over the years but the network protocol in this place doesn’t make that assumption. I need to look remember who we handled this case in other areas.

The bureaucracy is strong with that one.

6

BTW I am pretty sure I see the bug.

The file bkd_changes.c is the code that handles the bkd-side of running changes on a remote repository. The code that returns the results is at the end of cmd_chg_part1()

	f = popenvp(new_av, "r");
	out("@CHANGES INFO@\n");
	while (fnext(buf, f)) {
		if (newline) outc(BKD_DATA);
		newline = (strchr(buf, '\n') != 0);
		if (out(buf) <= 0) break;
	}
	pclose(f);

The ‘newline’ part of this loop is to handle when the ‘bk changes’ output is not newline terminated. It is incorrect when fnext() (an alias for fgets()) returns a non-newline terminated line because it doesn’t fit in the buffer. So any fix for this problem will need to fix this loop and the other copy of the same loop in part2.

Modern bk would use fgetline() which has no line length limitations, but the code in changes.c would then need to be fixed as well and that is harder because of the weird getline2() API.

7

OK. I have a working cset that handles arbitrary length lines and includes a testcase.

Will figure out how to make a new release with recent bugfixes.

8

I pushed a fix for this problem to bk://bkbits.net/bk/dev.

http://repos.bkbits.net/bk/dev/?PAGE=patch&REV=59b978c32qzeLS3ExKwd38b675kYMg

I will be working on a bk-7.3.2 release next week and this will be included.