#51083 Ticket 51082 - abort when a empty valueset is freed
Closed 3 years ago by spichugi. Opened 3 years ago by tbordaz.
tbordaz/389-ds-base ticket_51082  into  master

@@ -521,6 +521,63 @@ 

      log.info('Check the error log contains strings showing csn generator is tested')

      assert m1.searchErrorsLog("_csngen_gen_tester_main")

  

+ @pytest.mark.ds51082

+ def test_csnpurge_large_valueset(topo_m2):

+     """Test csn generator test

+ 

+     :id: 63e2bdb2-0a8f-4660-9465-7b80a9f72a74

+     :setup: MMR with 2 masters

+     :steps:

+         1. Create a test_user

+         2. add a large set of values (more than 10)

+         3. delete all the values (more than 10)

+         4. configure the replica to purge those values (purgedelay=5s)

+         5. Waiting for 6 second

+         6. do a series of update

+     :expectedresults:

+         1. Should succeeds

+         2. Should succeeds

+         3. Should succeeds

+         4. Should succeeds

+         5. Should succeeds

+         6. Should not crash

+     """

+     m1 = topo_m2.ms["master2"]

+ 

+     test_user = UserAccount(m1, TEST_ENTRY_DN)

+     if test_user.exists():

+         log.info('Deleting entry {}'.format(TEST_ENTRY_DN))

+         test_user.delete()

+     test_user.create(properties={

+         'uid': TEST_ENTRY_NAME,

+         'cn': TEST_ENTRY_NAME,

+         'sn': TEST_ENTRY_NAME,

+         'userPassword': TEST_ENTRY_NAME,

+         'uidNumber' : '1000',

+         'gidNumber' : '2000',

+         'homeDirectory' : '/home/mmrepl_test',

+     })

+ 

+     # create a large value set so that it is sorted

+     for i in range(1,20):

+         test_user.add('description', 'value {}'.format(str(i)))

+ 

+     # delete all values of the valueset

+     for i in range(1,20):

+         test_user.remove('description', 'value {}'.format(str(i)))

+ 

+     # set purging delay to 5 second and wait more that 5second

+     replicas = Replicas(m1)

+     replica = replicas.list()[0]

+     log.info('nsds5ReplicaPurgeDelay to 5')

+     replica.set('nsds5ReplicaPurgeDelay', '5')

+     time.sleep(6)

+ 

+     # add some new values to the valueset containing entries that should be purged

+     for i in range(21,25):

+         test_user.add('description', 'value {}'.format(str(i)))

+ 

+ 

  if __name__ == '__main__':

      # Run isolated

      # -s for DEBUG mode

@@ -801,6 +801,10 @@ 

              }

          }

      } else {

+         /* empty valueset - reset the vs->num so that further

+          * checking will not abort

+          */

+         vs->num = 0;

          slapi_valueset_done(vs);

      }

  

Bug Description:
A large valueset (more than 10 values) manages a sorted array of values.
replication purges old values from a valueset (valueset_array_purge). If it purges all the values
the valueset is freed (slapi_valueset_done).
A problem is that the counter of values, in the valueset, is still reflecting the initial number
of values (before the purge). When the valueset is freed (because empty) a safety checking
detects incoherent values based on the wrong counter.

Fix Description:
When all the values have been purge reset the counter before freeing the valueset

https://pagure.io/389-ds-base/issue/51082

Reviewed by: ?

Platforms tested: F30

Flag Day: no

Doc impact: no

ACK!

I've always been a bit worried about this code, it's had lot of issues, and we've applied fixes from community members without having any CI test to verify what was really being fixed, etc. So great to see a reproducer and fix.

rebased onto 0cb1e04

3 years ago

Pull-Request has been merged by tbordaz

3 years ago

Ack, great find @tbordaz :)

@mreynolds IMO the issue is we try to split sort or not sorting and that ads complication, when we should just always sort, and then these problems can't exist, because we also have a consistent structure, rather than this "maybe" thing.

Original design wants to keep the order of values in order to be smart with application ('Design' in http://www.port389.org/docs/389ds/design/static-group-performance.html).

I think it is mostly right, although in valueset_array_purge order may change (https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/valueset.c#_767). Note that it should only affects deleted values so change of order should rarely be noticed.

I think the original constraint of keeping order is valid. There are likely many applications, than nobody would want to change, that assume the order of values is chronological

Ldap specifies that ordering of attributes is always undefined,so I think we should not "offer this" as a "feature".

I agree that it is not a LDAP requirement however users of 389-ds are use to this. If we are changing the behavior between versions it could create issues to those users.

Also the "feature" already exists. It works great but of course it makes the code a bit more complex and bug prone. At the moment it is really robust and removing it will likely introduce new bugs. Also the extra cost of the "feature" is not significant (an array of slapi_value *) that is only updated with add at the end and remove compact.

The benefit of this remove would be to simplify the code.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4136

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago