Uploaded image for project: 'Sentry'
  1. Sentry
  2. SENTRY-1683

MetastoreCacheInitializer has a race condition in handling results list

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.0
    • Fix Version/s: 1.8.0
    • Component/s: Sentry
    • Labels:
      None

      Description

      The MetastoreCacheInitializer has the following logic:

      1) It creates a list of futures to hold results of executors (results)
      2) {{CreateInitialUpdate() }}does this:

        UpdateableAuthzPaths createInitialUpdate() throws
                Exception {
          PathsUpdate tempUpdate = new PathsUpdate(-1, false);
          List<String> allDbStr = hmsHandler.get_all_databases();
          for (String dbName : allDbStr) {
            Callable<CallResult> dbTask = new DbTask(tempUpdate, dbName);
            results.add(threadPool.submit(dbTask));
          }
      
          while (taskCounter.get() > 0) {
            Thread.sleep(1000);
            // Wait until no more tasks remain
          }
      
          for (Future<CallResult> result : results) {
            CallResult callResult = result.get();
      
            // Fail the HMS startup if tasks are not all successful and
            // fail on partial updates flag is set in the config.
            if (!callResult.getSuccessStatus() && failOnRetry) {
              throw callResult.getFailure();
            }
          }
      
          authzPaths.updatePartial(Lists.newArrayList(tempUpdate),
                  new ReentrantReadWriteLock());
          return authzPaths;
        }
      

      Note that in both cases the results list is accessed without holding any locks.
      But the list is also updated from tasks themselves:

        class DbTask extends BaseTask {
      
          @Override
          public void doTask() throws TException, SentryMalformedPathException {
           ...
            List<String> allTblStr = hmsHandler.get_all_tables(dbName);
            for (int i = 0; i < allTblStr.size(); i += maxTablesPerCall) {
              synchronized (results) {
                results.add(threadPool.submit(tableTask));
              }
            }
      

      There is some synchronization which uses taskCounter so the second walk of the list happens when all tasks are complete, but the first walk isn't thread-safe - the list may be updated while initial tasks are created there. Note that some access are synchronized and some are not.

        Attachments

        1. SENTRY-1683.001.patch
          3 kB
          Alex Kolbasov
        2. SENTRY-1683.001.patch
          2 kB
          Alex Kolbasov

          Issue Links

            Activity

              People

              • Assignee:
                akolb Alex Kolbasov
                Reporter:
                akolb Alex Kolbasov
              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: