#80 [PATCH] fixed buffer-paging issue in putsource()

[dima]

This patch fixes a bug where the block number and the block index would get out
of sync. Inside putsource() there was code that boiled down to this:

    do {
            if(...) {
                    Change = YES;
                    cp = blockp;
            }

            putline(output);
    } while (blockp != NULL && getrefchar() != '\n');
    if (Change == YES) blockp = cp;

It was possible for putline() to read in a new block. The last line could then
reset blockp WITHOUT reloading the old block. This effectively skipped a block's
worth of data resulting in a cscope report with some extra bogus data and some
missing valid data.

This is a very wide-ranging issue, and I'm certain that everybody who uses cscope regularly has hit it at least once. It's a bit of a corner case since a block boundary has to lie in just the right place, so it's not something people would see all the time. Ideally we'd move away from buffers entirely to something like mmap(), but that involves more upheaval that is worthwhile, probably...

[Hans-Bernhard Broeker]

Ticket moved from /p/cscope/bugs/286/

[dima]

I couldn't send a test case earlier because the code in question wasn't mine to
share. But now that I know what to look for, I have a test case from the cscope
codebase itself. This test case revealed a problem with the attached patch. I
will fix this, and send out a new patch. In the meatime, here's the test case:

I checked out the latest cscope sources from CVS on 2013-07-27. (Is there a
repo-wide 'cvs status' command instead of just per-file?) I then made a tiny
change:

dima@shorty:~/cscope$ cvs diff
Index: src/invlib.c
===================================================================
RCS file: /cvsroot/cscope/cscope/src/invlib.c,v
retrieving revision 1.21
diff -r1.21 invlib.c
306c306
<       if (fwrite(&zerolong, sizeof(zerolong), 1, outfile) == 0) {
---
>       if (fwrite(&zerolong, abcdef(zerolong), 1, outfile) == 0) {

Then I built a new cscope index:

dima@shorty:~/cscope$ rm -rf cscope.out; cscope-indexer -r -v
Creating list of files to index ...
Creating list of files to index ... done
Indexing files ...
Indexing files ... done

Then I ran a cscope query to find all functions called by invmake()

dima@shorty:~/cscope$ cscope -d -L2 invmake
src/invlib.c vpfopen 124 if ((outfile = vpfopen(invname, "w+b")) == NULL) {
src/invlib.c invcannotopen 125 invcannotopen(invname);
src/invlib.c fseek 129 fseek(outfile, BUFSIZ, SEEK_SET);
src/invlib.c vpfopen 132 if ((fpost = vpfopen(invpost, "wb")) == NULL) {
src/invlib.c invcannotopen 133 invcannotopen(invpost);
src/invlib.c malloc 139 if ((POST = malloc(postsize)) == NULL) {
src/invlib.c invcannotalloc 140 invcannotalloc(postsize);
src/invlib.c malloc 145 if ((SUPFING = malloc(supersize)) == NULL) {
src/invlib.c invcannotalloc 146 invcannotalloc(supersize);
src/invlib.c malloc 153 if ((SUPINT = malloc(supintsize * sizeof(long ))) == NULL) {
src/invlib.c invcannotalloc 154 invcannotalloc(supintsize * sizeof(long ));
src/invlib.c strcpy 160 strcpy(thisterm, "");
src/invlib.c fgets 179 while (fgets(line, TERMMAX, infile) != NULL) {
src/invlib.c strchr 183 s = strchr(line, SEP);
src/invlib.c strlen 191 if ((i = strlen(line)) > maxtermlen) {
src/invlib.c printf 196 printf("%ld: %s ", totpost, line);
src/invlib.c fflush 197 fflush(stdout);
src/invlib.c strcmp 199 if (strcmp(thisterm, line) == 0) {
src/invlib.c realloc 203 if ((POST = realloc(POST, postsize)) == NULL) {
src/invlib.c invcannotalloc 204 invcannotalloc(postsize);
src/invlib.c printf 209 printf("reallocated post space to %u, totpost=%ld\n",
src/invlib.c invnewterm 216 if (!invnewterm()) {
src/invlib.c strcpy 219 strcpy(thisterm, line);
src/invlib.c printf 249 printf("%ld %ld %ld %ld\n", posting.fileindex,
src/invlib.c fflush 251 fflush(stdout);
src/invlib.c invnewterm 254 if (!invnewterm()) {
src/invlib.c fwrite 262 if (fwrite(&logicalblk, sizeof(t_logicalblk), 1, outfile) == 0) {
src/invlib.c fwrite 267 if (fwrite(&logicalblk, sizeof(t_logicalblk), 1, outfile) == 0) {
src/invlib.c fwrite 278 if (fwrite(SUPINT, sizeof(*SUPINT), numlogblk + 1, outfile) == 0 ||
src/invlib.c fwrite 279 fwrite(SUPFING, 1, supfing - SUPFING, outfile) == 0) {
src/invlib.c fwrite 289 if (fwrite(temp, sizeof(t_logicalblk) - i, 1, outfile) == 0 ||
src/invlib.c fflush 290 fflush(outfile) == EOF) {
src/invlib.c rewind 294 rewind(outfile);
src/invlib.c fwrite 302 if (fwrite(&param, sizeof(param), 1, outfile) == 0) {
src/invlib.c fwrite 306 if (fwrite(&zerolong, abcdef(zerolong), 1, outfile) == 0) {

----- look here -----

src/invlib.c fread 638 fread(invcntl->iindex, (int ) invcntl->param.supsize, 1,
src/invlib.c boolready 642 if (boolready() == -1) {
src/invlib.c fclose 644 fclose(invcntl->postfile);
src/invlib.c fclose 646 fclose(invcntl->invfile);
src/invlib.c rewind 652 rewind(invcntl->invfile);
src/invlib.c fwrite 653 fwrite(&invcntl->param, sizeof(invcntl->param), 1, invcntl->invfile);

Note that the abcdef() call is missing. Also note that the reported calls
immediately after where the abcdef() call should have been are no longer in the
invmake() function. For instance the fread() call on line 638 is in invopen()
not invmake().

As I said earlier, the patch appears to be flawed. After I apply the patch in
this report, cscope goes into an infinite loop. This patch did work for my
earlier test case, but it's clearly insufficient, and I'll send a new one out
shortly.

[dima]

OK. Fixed it.

It turned out that my previous fix tickled another bug lurking in
that area of the code.

I was sloppy in the exact codebase I was applying cscope to, and
I can no longer obtain the buggy behavior with the directions
above. Thus I'm doing this again, using the unmodified cscope
15.8a tarball from sourceforge as the source repository to apply
cscope to. To build the index I ran

 cscope-indexer -r -v

I'm attaching this index file as well. With this index file and
this source I see the following 3 behaviors:

1. stock cscope 15.8a

   dima@shorty:~/cscope$ cscope -d -L2 loadmenu

   src/mouse.c mousereinit 174 mousereinit();
   src/mouse.c printf 175 (void ) printf("\033V1");

2. cscope 15.8a with just the patch above

   infinite loop

3. cscope 15.8a with the two patches in this message

   dima@shorty:~/cscope$ cscope -d -L2 loadmenu

   src/mouse.c mousereinit 174 mousereinit();
   src/mouse.c printf 175 (void ) printf("\033V1");
   src/mouse.c printf 176 (void ) printf("\033M0@%s@%s@", menu[0].text, menu[0].value);
   src/mouse.c printf 178 (void ) printf("\033M@%s@%s@", menu[i].text, menu[i].value);
   src/mouse.c mousecleanup 184 mousecleanup();
   src/mouse.c printf 185 (void ) printf("\033[6;1X\033[9;1X");
   src/mouse.c strlen 187 len = strlen(menu[i].text);
   src/mouse.c printf 188 (void ) printf("\033[%d;%dx%s%s", len,
   src/mouse.c strlen 189 (int ) (len + strlen(menu[i].value)),
   src/mouse.c fflush 194 (void ) fflush(stdout);

You can confirm that #3 is correct. The bug fixed in the second
patch is described in its commit log (at the top of the patch
file). It's nothing particularly interesting.

dima

[dima]

Patches