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

Memcached should disallow DCP connections with names greater than 200 characters (was: ns_server connections may be terminated if a client uses a long DCP_OPEN.name)

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 6.0.0, 5.5.0, 5.5.1, 5.5.2, 5.5.3, 6.0.1, 6.0.2, 5.5.4
    • Fix Version/s: 7.0.0
    • Component/s: memcached
    • Labels:
    • Sprint:
      KV-Engine MH 2nd Beta

      Description

      Summary

      If a DCP client uses a long (exact length TBD) name when connecting, it can result in
      STATS calls (for example performed by ns_server to monitor rebalance) to fail:

       462445 2019-05-11T08:18:28.525392+00:00 WARNING 143: exception occurred in runloop during packet execution. Cookie info: {"packet":{"magic":"ClientRequest","opcode":"STAT","keylen":3,"extlen":0,"datatype":"raw","vbucket":0,"bodylen":3,"opaque":"0x0","cas":0},"connection":"[ 127.0.0.1:48735 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]"} - closing connection: Response::setKeylen: key cannot exceed 1 byte 
      

      Details

      Looks like problem is here - we cap the response keyLen to 1 byte for all response types, not just "Response with flex-framing extras" (0x18):

          void setKeylen(uint16_t value) {
              if (value > 0xff) {
                  throw std::invalid_argument(
                          "Response::setKeylen: key cannot exceed 1 byte");
              }
              keylen = uint8_t(value);
          }
      

      This change was introduced via: http://review.couchbase.org/#/c/86393/

      See https://github.com/couchbase/kv_engine/blob/master/docs/BinaryProtocol.md#response-header - it should be permitted to return keys up to 65,535 bytes long for "regular" response packets (0x81).

        Attachments

          Issue Links

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

            Activity

            Hide
            drigby Dave Rigby added a comment -

            Raju Suravarjjala / Wayne Siu requesting for 6.0.3 (note this was initially targeted for 6.0.2 but moved out as too late).

            Show
            drigby Dave Rigby added a comment - Raju Suravarjjala / Wayne Siu requesting for 6.0.3 (note this was initially targeted for 6.0.2 but moved out as too late).
            Hide
            dfinlay Dave Finlay added a comment - - edited

            We discovered this issue on a cluster where the Elastic Search connector connected to a cluster and ns_server "control" connections to memcached started dropping. The elastic search connector has nothing to do with ns_server DCP connections, however, ns_server does monitor the DCP replications it manages and the "dcp" stats command started failing as it retrieves information for all DCP stats, not just ns_server DCP replications. Separately, memcached should not allow DCP clients to connect with connection names that are long enough to cause a subsequent stats call to fail. And lastly clients should know about the name length limitation and ensure that their connection names are less than the limit.

            I would like to fix this issue in the following ways.

            1. Memcached should define the max length of a DCP stat name and disallow DCP connections to be established with names that have a length > 255 - MAX_DCP_STAT_NAME_LENGTH. Currently it looks like the longest stat name is stream_1024_cur_snapshot_prepare, which is 33 characters long. (This ticket now tracks this improvement)
            2. Memcached clients, including ns_server should modify the way their DCP connection names are constructed to ensure they are always less than the max allowed name. (Tracked by MB-35881.)
            3. Lastly, it seems like a good idea to enhance the "dcp" stats call to take an optional connection name prefix so that ns_server can query stats only on the connections it has set up. (Tracked by MB-35882).

            I'll file tickets on these issues and link here.

            Show
            dfinlay Dave Finlay added a comment - - edited We discovered this issue on a cluster where the Elastic Search connector connected to a cluster and ns_server "control" connections to memcached started dropping. The elastic search connector has nothing to do with ns_server DCP connections, however, ns_server does monitor the DCP replications it manages and the "dcp" stats command started failing as it retrieves information for all DCP stats, not just ns_server DCP replications. Separately, memcached should not allow DCP clients to connect with connection names that are long enough to cause a subsequent stats call to fail. And lastly clients should know about the name length limitation and ensure that their connection names are less than the limit. I would like to fix this issue in the following ways. Memcached should define the max length of a DCP stat name and disallow DCP connections to be established with names that have a length > 255 - MAX_DCP_STAT_NAME_LENGTH. Currently it looks like the longest stat name is stream_1024_cur_snapshot_prepare , which is 33 characters long. (This ticket now tracks this improvement) Memcached clients, including ns_server should modify the way their DCP connection names are constructed to ensure they are always less than the max allowed name. (Tracked by MB-35881 .) Lastly, it seems like a good idea to enhance the "dcp" stats call to take an optional connection name prefix so that ns_server can query stats only on the connections it has set up. (Tracked by MB-35882 ). I'll file tickets on these issues and link here.
            Hide
            trond Trond Norbye added a comment -

            I took a quick look in mad hatter and I found at least one stats there which is 38 bytes long (plus the prefix: "eq_dcp:" and ":" after the name) which means we're (at least) around 46 characters. I'm tempted to set the max DCP name to 200 characters (looks better than a limit of lets say 207 characters )

            Show
            trond Trond Norbye added a comment - I took a quick look in mad hatter and I found at least one stats there which is 38 bytes long (plus the prefix: "eq_dcp:" and ":" after the name) which means we're (at least) around 46 characters. I'm tempted to set the max DCP name to 200 characters (looks better than a limit of lets say 207 characters )
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-7.0.0-1022 contains kv_engine commit 94b4570 with commit message:
            MB-34280: Set max DCP name to 200 characters

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-7.0.0-1022 contains kv_engine commit 94b4570 with commit message: MB-34280 : Set max DCP name to 200 characters
            Hide
            owend Daniel Owen added a comment -

            Release note
            A new restriction has been added where a DCP connection name cannot exceed 200 characters.
            This restriction applies to all DCP connection requests to the data service (memcached).

            Show
            owend Daniel Owen added a comment - Release note A new restriction has been added where a DCP connection name cannot exceed 200 characters. This restriction applies to all DCP connection requests to the data service (memcached).
            Hide
            david.nault David Nault added a comment -

            Hi Dave Rigby, can you clarify whether the limit is 200 characters, or 200 bytes?

            Show
            david.nault David Nault added a comment - Hi Dave Rigby , can you clarify whether the limit is 200 characters, or 200 bytes?
            Hide
            drigby Dave Rigby added a comment -

            Bytes (pretty much everything in memcached binary protocol is byte counts)

            Show
            drigby Dave Rigby added a comment - Bytes (pretty much everything in memcached binary protocol is byte counts)

              People

              Assignee:
              trond Trond Norbye
              Reporter:
              drigby Dave Rigby
              Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Gerrit Reviews

                  There are no open Gerrit changes

                    PagerDuty