Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-41944

Item::removeUserXattrs doesn't take a copy of the blob

    XMLWordPrintable

    Details

    • Triage:
      Triaged
    • Story Points:
      1
    • Is this a Regression?:
      Yes

      Description

      Summary

      The code path in Item::removeUserXattrs() (added in 6.6.0 for MB-37374) may corrupt the item value in memory. That may lead to a number of issues including crashes, DCP disconnection, corrupted data being written to disk in the payload of deletions, corrupted data being streamed via DCP in the payload of deletions.

      The most likely scenario for hitting the issue seems when we have tombstones with SysXattrs (eg, SyncGateway) and UserXattrs in the payload. That usually ends up in a crash.

      Details

      Before 6.6, a deletion in memcached could never contain UserXattrs in the payload. Only SysXattrs were allowed (eg, Mobile Convergence - the "_sync" SysXattr stored by SyncGateway).
      From 6.6 memcached allows UserXattrs in deletions. That is for supporting "Transparent Transactions" (TXNJ-125). The MCBP now exposes a new CreateAsDeleted API. The SDK uses it for creating "staged" docs for TX as tombstones. The "staged" payload is stored in the "txn" UserXattr in a tombstone.

      A Consumer connection to a 6.6 memcached node must enable the IncludeDeletedUserXattrs feature for getting UserXattrs in deletions over that connection.
      For backward compatibility, and for supporting components that don't understand UserXattrs in deletions, the IncludeDeletedUserXattrs feature is disabled by default on DCP Producers.
      If memcached stores a deletions with UserXattrs in the payload, then 2 things may happen at stream over DCP:

      1. IncludeDeletedUserXattrs is enabled. Nothing to do here, we just stream the deletion as it is
      2. IncludeDeletedUserXattrs is disabled. Before streaming, we need to check if the payload contains any UserXattrs and we remove it before streaming.

      Item::removeUserXattrs() is when (2) happens.
      For supporting multiple DCP connections with different IncludeDeletedUserXattrs setting, we need to operate on a copy of the payload when making any change to it. Any direct change to the payload would reflect on unrelated DCP connections (and possibly on disk) otherwise, which is what happens here.

      What do we need for hitting the issue?

      If we consider only the SDK and memcached during a transaction then we are fine.

      1. The SDK stages a doc via a Sync Subdoc(CreateAsDeleted) -> a SyncDelete with a "txn" UserXattr is stored and processed in memcached
      2. memcached processes the SyncDelete to completion -> the SyncDelete first and then the committed Delete are streamed over a DCP replica connection that supports IncludeDeletedUserXattrs, so don't even execute Item::removeUserXattrs().

      But if we introduce a component (eg, SyncGateway, GSI, etc..) that streams from memcached via DCP and doesn't enable IncludeDeletedUserXattrs, then we execute Item::removeUserXattrs() on that connection for any deletion that contains UserXAttrs. If that happens, then we modify the shared payload stored in memory, which is the payload pointed by the persistence and replication queue (CheckpointManager).
      At that point, any component that processes the same deletion again from the CheckpointManger will likely hit some issue. Eg, crash at replication when we validate the corrupted payload, persistence of corrupted data (if the flusher persists the deletion after Item::removeUserXattrs() has executed for that deletion), etc..

       

      Why exactly we may see a crash?

      1. TX completes and creates a committed doc with a given Body
      2. SG updates the doc by adding the "_sync" SysXattrs -> doc is alive with Body+SysXattr
      3. The user deletes the doc via CMD_DEL -> now we have a tombstone with SysXattr only
      4. TX again on the same doc (ie, repeat step (1)) -> TX issues a CreateAsDelete and we end up with a (committed) tombstone with the pre-existing SysXattr + the new "txn" UserXattr
      5. We stream the committed tombstone over a DCP connection that triggers Item::removeUserXattrs() -> the result is that we remove the UserXattrs and we update the Xattr-block size in the shared blob for that item
      6. A second DCP connection triggers Item::removeUserXattrs() -> At the previous step we have overwritten the size of the Xattr-block in the payload, which is used for computing the size of the Body-chunk -> In Item::removeUserXattrs() now we find that the Body size for the deletion being processed is > 0 -> Invalid, we trigger an exception and crash

       

      Do we strictly need SyncGateway for hitting a crash?

      Answer is no. SyncGateway introduces the "_sync" SysXAttr (also in deletions). That together with TX leads memcached to corrupting data in a way that leads to a crash. The point here is on having a SysXAttr + a UserXattr in the payload of a deletion, it's not SyncGateway itself.

       

      If we remove SysXattrs from the described scenario, can we still hit any issue?

      I have tried to crash the node by removing SyncGateway and adding just a GSI index that covers my data. I didn't manage to crash the node and I didn't manage to get any corruption in-memory/on-disk either.

       

      Do we have a workaround?

      For avoiding the issue in 6.6 we should avoid UserXattrs in deletions.
      Given that TX-1.1 seems the only component that creates tombstones with UserXattrs, using TX-1.0 in place of TX-1.1 on 6.6 cluster is a possibility. Note that here I'm just sharing technical considerations, I am aware that TX-1.1 contains also some fixes (apart from the usage of the newest CreateAsDeleted). The SDK team is best suited for providing an insight on this.

        Attachments

          Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

            Activity

            Hide
            paolo.cocchi Paolo Cocchi added a comment -

            Hey Ashwin Govindarajulu, opinion in KV is that we can close this as the issue is fixed and verified by QE, plus your latest test is not failing at fix. Assigning back to you, thanks.

            Show
            paolo.cocchi Paolo Cocchi added a comment - Hey Ashwin Govindarajulu , opinion in KV is that we can close this as the issue is fixed and verified by QE, plus your latest test is not failing at fix. Assigning back to you, thanks.
            Hide
            ashwin.govindarajulu Ashwin Govindarajulu added a comment -

            Thanks Paolo Cocchi and Graham Pople.

            Closing this ticket since we are not seeing this issue on 6.6.1-9132.

            Show
            ashwin.govindarajulu Ashwin Govindarajulu added a comment - Thanks Paolo Cocchi and Graham Pople . Closing this ticket since we are not seeing this issue on 6.6.1-9132.
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-6.6.1-9152 contains kv_engine commit 725c864 with commit message:
            MB-41944: Improve logging for body-size>0 in Item::removeUserXattrs

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-6.6.1-9152 contains kv_engine commit 725c864 with commit message: MB-41944 : Improve logging for body-size>0 in Item::removeUserXattrs
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-7.0.0-3683 contains kv_engine commit 897cd88 with commit message:
            MB-41944: Item::removeUserXattrs() operates on a copy

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-7.0.0-3683 contains kv_engine commit 897cd88 with commit message: MB-41944 : Item::removeUserXattrs() operates on a copy
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-7.0.0-3700 contains kv_engine commit 725c864 with commit message:
            MB-41944: Improve logging for body-size>0 in Item::removeUserXattrs

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-7.0.0-3700 contains kv_engine commit 725c864 with commit message: MB-41944 : Improve logging for body-size>0 in Item::removeUserXattrs

              People

              Assignee:
              drigby Dave Rigby
              Reporter:
              paolo.cocchi Paolo Cocchi
              Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Gerrit Reviews

                  There are no open Gerrit changes

                    PagerDuty