#47779 Part of DNA shared configuration is deleted after server restart
Closed: Fixed None Opened 5 years ago by nhosoi.

Description of problem:
When the server with DNA shared configuration entries (MMR + DNA setup) has set
dnaRemoteBindMethod and dnaRemoteConnProtocol attributes in its shared entry
and is restarted, the configuration is being deleted by an update event
triggered some time after DNA plugin initialization.

log:
[07/Mar/2014:13:48:01 -0500] NS7bitAttr - ADD begin
[07/Mar/2014:13:48:01 -0500] NS7bitAttr - ADD
target=dnaHostname=example.com+dnaPortNum=1389,cn=Account
UIDs,ou=Ranges,dc=example,dc=com
[07/Mar/2014:13:48:01 -0500] NSMMReplicationPlugin - changelog program -
cl5WriteOperationTxn: successfully written entry with csn
(531a1462000200070000)

Version-Release number of selected component (if applicable):
389-ds-base-1.3.1.6-21.el7

How reproducible:
always

Steps to Reproduce:
1. Set up MMR + DNA.
2. Configure dnaRemoteBindMethod and dnaRemoteConnProtocol in the shared entry
on one of the masters.
3. Restart the directory server instance that owns the entry.

Actual results:
The attributes are deleted from the shared configuration entry in less than a
minute after the server starts.

Expected results:
The configuration is not affected by server restart.


Your design looks good. One concern is a deadlock possibility.

dna_update_shared_config holds dna_server_read_lock, then updates a shared config entry by slapi_add_internal_pb(pb) in which it acquires the backend lock, then releases the lock.

On the other hand, dna_config_check_post_op calls dna_load_shared_servers, in which it tries to acquire dna_server_write_lock. Please note that dna_config_post_op is called at the be_txn_post timing. That is, in the backend lock...

You may need to call slapi_back_transaction_begin before calling dna_server_read_lock in dna_update_shared_config... Please see also dna_update_config_event.

Replying to [comment:4 nhosoi]:

Your design looks good. One concern is a deadlock possibility.

dna_update_shared_config holds dna_server_read_lock, then updates a shared config entry by slapi_add_internal_pb(pb) in which it acquires the backend lock, then releases the lock.

On the other hand, dna_config_check_post_op calls dna_load_shared_servers, in which it tries to acquire dna_server_write_lock. Please note that dna_config_post_op is called at the be_txn_post timing. That is, in the backend lock...

You may need to call slapi_back_transaction_begin before calling dna_server_read_lock in dna_update_shared_config... Please see also dna_update_config_event.

I revised the fix so that we do not hold the dna server rwlock while updating the database. Since the bind method and protocol never exceed 15 characters, I used fixed buffers to store the values when re-adding the shared config entry.

New patch is attached...

It's ok to use PR_snprintf in this case, even though you are not doing a conversion, because this is not performance sensitive code. In the future, and for performance sensitive code, use PL_strncpyz, which is guaranteed to null terminate the string buffer, if you are not doing a format conversion.

git merge ticket47779
Updating 136fa64..06a3d0a
Fast-forward
ldap/servers/plugins/dna/dna.c | 218 ++++++++++++++++++++++++++++++++++++++--
1 files changed, 208 insertions(+), 10 deletions(-)

git push origin master
136fa64..06a3d0a master -> master

commit 06a3d0a
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Apr 21 11:16:09 2014 -0400

821297d..97412d6 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 97412d6

e97f2b8..eb6ea36 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit eb6ea3642a0e145d3a0d7de4bbca7c05cde7a3d3

Replying to [comment:7 rmeggins]:

It's ok to use PR_snprintf in this case, even though you are not doing a conversion, because this is not performance sensitive code. In the future, and for performance sensitive code, use PL_strncpyz, which is guaranteed to null terminate the string buffer, if you are not doing a format conversion.

Sorry committed the change before I saw this message. Thanks for the insight!

Just a question about protecting dna_delete_global_servers.
In be_postop of the config it calls dna_config_check_post_op->dna_load_shared_servers->dna_delete_global_servers without holding g_dna_cache_server_lock.
If at the same time dna_get_shared_config_attr_val is called, I wonder if it may access a freed dna_global_servers.

It could be the same issue in dna_stop where dna_delete_global_servers without holding g_dna_cache_server_lock

Replying to [comment:10 tbordaz]:

Just a question about protecting dna_delete_global_servers.
In be_postop of the config it calls dna_config_check_post_op->dna_load_shared_servers->dna_delete_global_servers without holding g_dna_cache_server_lock.
If at the same time dna_get_shared_config_attr_val is called, I wonder if it may access a freed dna_global_servers.

It could be the same issue in dna_stop where dna_delete_global_servers without holding g_dna_cache_server_lock

Yeah dna_delete_global_servers() needs to be protected. I'll work on a new patch.

Thanks!

Replying to [comment:11 mreynolds]:

Replying to [comment:10 tbordaz]:

Just a question about protecting dna_delete_global_servers.
In be_postop of the config it calls dna_config_check_post_op->dna_load_shared_servers->dna_delete_global_servers without holding g_dna_cache_server_lock.
If at the same time dna_get_shared_config_attr_val is called, I wonder if it may access a freed dna_global_servers.

It could be the same issue in dna_stop where dna_delete_global_servers without holding g_dna_cache_server_lock

Yeah dna_delete_global_servers() needs to be protected. I'll work on a new patch.

Thanks!

Also, the list does not need to be locked during the close function. All active threads should be stopped(during server shutdown) before calling the plugin close functions. And in 1.3.3, the dynamic plugin feature will also safely handle the close function.

New patch attached.

The fix looks good to me. Thanks

git merge ticket47779
Updating ab84ab8..2b98dca
Fast-forward
ldap/servers/plugins/dna/dna.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

git push origin master
ab84ab8..2b98dca master -> master

commit 2b98dca
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Apr 28 11:02:14 2014 -0400

bdb6962..9cf2aa3 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 9cf2aa3

4b986ee..f94bfa1 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit f94bfa1917c95f2e93e43fb78c57a6cea96df69c

This fix can cause a timing related deadlock at server startup, reopening...

git merge ticket47779
Updating 46a3269..badd354
Fast-forward
ldap/servers/plugins/dna/dna.c | 113 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 91 insertions(+), 22 deletions(-)

git push origin master
46a3269..badd354 master -> master

commit badd354
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Jul 2 16:11:49 2014 -0400

a82c409..a696931 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit a696931

51a76a4..54bf43d 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit 54bf43d9e696368c73f5be71dd88d6e0c8f97dc0

Metadata Update from @mreynolds:
- Issue assigned to mreynolds
- Issue set to the milestone: 1.3.1.23

2 years ago

Login to comment on this ticket.

Metadata