ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-319

add locking around auth info in zhandle_t

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 3.0.1, 3.1.0
    • Fix Version/s: 3.1.1, 3.2.0
    • Component/s: c client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Looking over the zookeeper.c code it appears to me that the zoo_add_auth() function may be called at any time by the user in their "main" thread. This function alters the elements of the auth_info structure in the zhandle_t structure.

      Meanwhile, the IO thread may read those elements at any time in such functions as send_auth_info() and auth_completion_func(). It seems important, then, to add a lock which prevents data being read by the IO thread while only partially changed by the user's thread. The attached patch add such a lock.

      1. ZOOKEEPER-319.patch
        5 kB
        Chris Darroch
      2. ZOOKEEPER-319.patch
        5 kB
        Mahadev konar
      3. ZOOKEEPER-319.patch
        5 kB
        Chris Darroch

        Activity

        Hide
        Chris Darroch added a comment -

        This version avoids holding the auth lock while calling the user's auth completion function (which may run for a long time; we don't know).

        Show
        Chris Darroch added a comment - This version avoids holding the auth lock while calling the user's auth completion function (which may run for a long time; we don't know).
        Hide
        Mahadev konar added a comment -

        just a minor modification. my compiler fails on compiliing
        consta char* auth_data without initialization.

        +1 to the patch.

        one minor nit –

        • we have some logging in auth_completion_func() in zookeeper.c
         zoo_lock_auth(zh);
            
            if(rc!=0){
                LOG_ERROR(("Authentication scheme %s failed. Connection closed.",
                        zh->auth.scheme));
                zh->state=ZOO_AUTH_FAILED_STATE;
            }else{
                zh->auth.state=1;  // active
                LOG_INFO(("Authentication scheme %s succeeded", zh->auth.scheme));
            }
        
            if (zh->auth.completion) {
                auth_completion = zh->auth.completion;
                auth_data = zh->auth.data;
                zh->auth.completion=0;
            }
        
            zoo_unlock_auth(zh);
        

        Should we get rid of logging within the lock?

        Show
        Mahadev konar added a comment - just a minor modification. my compiler fails on compiliing consta char* auth_data without initialization. +1 to the patch. one minor nit – we have some logging in auth_completion_func() in zookeeper.c zoo_lock_auth(zh); if(rc!=0){ LOG_ERROR(("Authentication scheme %s failed. Connection closed.", zh->auth.scheme)); zh->state=ZOO_AUTH_FAILED_STATE; }else{ zh->auth.state=1; // active LOG_INFO(("Authentication scheme %s succeeded", zh->auth.scheme)); } if (zh->auth.completion) { auth_completion = zh->auth.completion; auth_data = zh->auth.data; zh->auth.completion=0; } zoo_unlock_auth(zh); Should we get rid of logging within the lock?
        Hide
        Chris Darroch added a comment -

        Good points – see if this suits.

        Show
        Chris Darroch added a comment - Good points – see if this suits.
        Hide
        Mahadev konar added a comment -

        I just committed this. thanks chris.

        Show
        Mahadev konar added a comment - I just committed this. thanks chris.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #243 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/243/)
        . add locking around auth info in zhandle_t. (chris darroch via mahadev)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #243 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/243/ ) . add locking around auth info in zhandle_t. (chris darroch via mahadev)

          People

          • Assignee:
            Chris Darroch
            Reporter:
            Chris Darroch
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development