Uploaded image for project: 'WiredTiger'
  1. WiredTiger
  2. WT-4909

Fix potential code caching/re-ordering issue in __search_insert_append

    XMLWordPrintable

    Details

    • Type: Technical Debt
    • Status: Closed
    • Priority: Minor - P4
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.2.1, WT3.2.1, 4.3.1
    • Component/s: None
    • Labels:
      None
    • Story Points:
      2
    • Sprint:
      Storage Engines 2019-07-29

      Description

      My reading of this code in __search_insert_append():

      if ((ins = WT_SKIP_LAST(ins_head)) == NULL)
              return (0);
      key.data = WT_INSERT_KEY(ins);
      key.size = WT_INSERT_KEY_SIZE(ins);
       
      WT_RET(__wt_compare(session, collator, srch_key, &key, &cmp));
      if (cmp >= 0) {
              /*
               * !!!
               * We may race with another appending thread.
               *
               * To catch that case, rely on the atomic pointer read above
               * and set the next stack to NULL here.  If we have raced with
               * another thread, one of the next pointers will not be NULL by
               * the time they are checked against the next stack inside the
               * serialized insert function.
               */
              for (i = WT_SKIP_MAXDEPTH - 1; i >= 0; i--) {
                      cbt->ins_stack[i] = (i == 0) ? &ins->next[0] :
                          (ins_head->tail[i] != NULL) ?
                          &ins_head->tail[i]->next[i] : &ins_head->head[i];
                      cbt->next_stack[i] = NULL;
              }
              cbt->compare = -cmp;
              cbt->ins = ins;
              cbt->ins_head = ins_head;
              *donep = 1;
      }
      

      worries me. I think we require ins be assigned before it's used in the if clause, and while it's vanishingly unlikely that wouldn't happen, there's nothing that stops the compiler from repeating the read inside the if clause, and I think that would cause a problem.

      cc Michael Cahill, Haribabu Kommi

        Attachments

          Activity

            People

            Assignee:
            alex.cameron Alex Cameron (Inactive)
            Reporter:
            keith.bostic Keith Bostic
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: