Uploaded image for project: 'XWiki Platform'
  1. XWiki Platform
  2. XWIKI-25766

Group cache can keep a stale recursive membership when a group is modified while it's being resolved

    XMLWordPrintable

Details

    • Bug
    • Resolution: Unresolved
    • Major
    • None
    • 17.10.15
    • User
    • None
    • Unknown

    Description

      The "all" (recursive) entry of MemberGroupsCache can be cached with a stale value when a group is modified while that entry is being computed, and it then stays wrong until the entry is invalidated for another reason or evicted.

      An entry is only invalidated through the references it's indexed under: its own reference when it's created, and the references of its value when setDirect/setAll is called. For the "all" entry, the groups two levels up or more are only indexed by the final setAll, after they have been read from the database.

      Scenario, with M ∈ G1 ∈ G2 ∈ G3 and a thread resolving the recursive groups of M (getGroups(M, wikis, true), or GroupManager#isMember(M, X, true) when M is not in X):

      1. The thread creates the entry E of M (indexed under M), sets its direct groups {G1} (indexed under G1) and reads the direct groups of G2: {G3}.
      2. An admin removes G2 from G3. GroupCacheInvalidationListener cleans G3 and G2, which removes the entries of G2 and G1. E is not indexed under G2 or G3 yet, so it stays in the cache.
      3. The thread calls E.setAll(G1, G2, G3): M is now considered a member of G3 until E is invalidated or evicted.

      The opposite case (a group added to another group during the resolution) caches a stale "not a member".

      A related problem in AbstractGroupCache: cacheDocumentIndex holds plain HashSet instances that addToIndex (called from setDirect/setAll) and cleanIndex (called from eviction) modify without the cache lock, so they can be modified concurrently.

      Possible fix: an invalidation counter in AbstractGroupCache, incremented by cleanCache/removeAll under the write lock, read before the first database read, and checked under the read lock before setDirect/setAll, the value not being cached when it changed. For the index, use concurrent sets (ConcurrentHashMap.newKeySet()).

      Found during the review of https://github.com/xwiki/xwiki-platform/pull/7431 (see https://github.com/xwiki/xwiki-platform/pull/7431#discussion_r4565355131).

      Attachments

        Issue Links

          Activity

            People

              Unassigned Unassigned
              vmassol Vincent Massol
              Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

                Created:
                Updated: