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

None
open
nobody
None
5
2013-07-30
2013-07-28
dima
No

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...

1 Attachments

Discussion

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

     
  • dima
    dima
    2013-07-29

    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
    dima
    2013-07-30

    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

     
    Attachments