#50079 Fix for ticket 50059: If an object is nsds5replica, it must be cn=replica
Closed 3 years ago by spichugi. Opened 5 years ago by gparente.
gparente/389-ds-base ticket50059  into  master

@@ -188,6 +188,11 @@ 

                                   CONFIG_FILTER, replica_config_post_modify);

  }

  

+ #define MSG_NOREPLICARDN "no replica rdn\n"

+ #define MSG_NOREPLICANORMRDN  "no replica normalized rdn\n"

+ #define MSG_CNREPLICA "replica rdn %s should be %s\n"

+ #define MSG_ALREADYCONFIGURED "replica already configured for %s\n"

+ 

  static int

  replica_config_add(Slapi_PBlock *pb __attribute__((unused)),

                     Slapi_Entry *e,
@@ -199,15 +204,48 @@ 

      Replica *r = NULL;

      multimaster_mtnode_extension *mtnode_ext;

      char *replica_root = (char *)slapi_entry_attr_get_charptr(e, attr_replicaRoot);

-     char buf[SLAPI_DSE_RETURNTEXT_SIZE];

-     char *errortext = errorbuf ? errorbuf : buf;

+     char *errortext = NULL;

+     Slapi_RDN *replicardn;

  

-     if (errorbuf) {

-         errorbuf[0] = '\0';

+     if (errorbuf != NULL) {

+         errortext = errorbuf;

      }

  

      *returncode = LDAP_SUCCESS;

  

+     /* check rdn is "cn=replica" */

+     replicardn = slapi_rdn_new_sdn(slapi_entry_get_sdn(e));

+     if (replicardn) {

+           char *nrdn = slapi_rdn_get_nrdn(replicardn);

+           if (nrdn == NULL) {

+               if (errortext != NULL) {

+                  strcpy(errortext, MSG_NOREPLICANORMRDN);

+               }

+               slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "replica_config_add - "MSG_NOREPLICANORMRDN);

+               slapi_rdn_free(&replicardn);

+               *returncode = LDAP_UNWILLING_TO_PERFORM;

+               return SLAPI_DSE_CALLBACK_ERROR;

+           } else {

+              if (strcmp(nrdn,REPLICA_RDN)!=0) {

+                  if (errortext != NULL) {

+                      PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE,MSG_CNREPLICA, nrdn, REPLICA_RDN);

+                  }

+                  slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name,"replica_config_add - "MSG_CNREPLICA, nrdn, REPLICA_RDN);

+                  slapi_rdn_free(&replicardn);

+                  *returncode = LDAP_UNWILLING_TO_PERFORM;

+                  return SLAPI_DSE_CALLBACK_ERROR;

+              }

+              slapi_rdn_free(&replicardn);

+           }

+     } else {

+         if (errortext != NULL) {

+             strcpy(errortext, MSG_NOREPLICARDN);

+         }

+         slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "replica_config_add - "MSG_NOREPLICARDN);

+         *returncode = LDAP_UNWILLING_TO_PERFORM;

+         return SLAPI_DSE_CALLBACK_ERROR;

+     }

+ 

      PR_Lock(s_configLock);

  

      /* add the dn to the dn hash so we can tell this replica is being configured */
@@ -217,8 +255,10 @@ 

      PR_ASSERT(mtnode_ext);

  

      if (mtnode_ext->replica) {

-         PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "replica already configured for %s", replica_root);

-         slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "replica_config_add - %s\n", errortext);

+         if ( errortext != NULL ) {

+             PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, MSG_ALREADYCONFIGURED, replica_root);

+         }

+         slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "replica_config_add - "MSG_ALREADYCONFIGURED, replica_root);

          *returncode = LDAP_UNWILLING_TO_PERFORM;

          goto done;

      }

Bug Description:

We should enforce that if an object is of type nsds5replica, it must be named cn=replica.
This has caused some confusion where people have misconfigured their system by trying alternate names.

Fix Description:

Check that rdn of replica dn is exactly REPLICA_RDN

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

Author: German Parente gparente@redhat.com

Review by: ???

If you have the bandwidth it would be great to add a testcase.

Nice patch.
An other option is to do the test before acquiring s_configLock and return LDAP_UNWILLING_TO_PERFORM immediately if it does not match REPLICA_RDN.
Note that in that case replica_root should be initialized only after that test

You should also the returntext so the client knows why the op was rejected

Same thing here, please set the returntext. Thanks!

rebased onto eb7ee0ec726b78a7983d88a7eb7461439ff825cf

5 years ago

Thanks for all the remarks. I have update the fix. I still need to create the unit test.

The patch is good to me.

Why do we need to strcpy into errortext? The error isn't passed down to any other if statement as we immediately return. As a result, this seems like we should just have the error text straight into the log.

Why do we need to strcpy into errortext? The error isn't passed down to any other if statement as we immediately return. As a result, this seems like we should just have the error text straight into the log.

thanks William for the comments. It seemed to me that errortext = errorbuf is a parameter of this function registered as callback. I thought it is used to return as text to the client application. But perhaps I am wrong ? But if errorbuf is NULL, I agree errortext has no sense but local to the function.

You're right, what it's doing is is setting errortxt as buf OR a stack buffer. I think that ternaries are the most evil piece of code existing, because it masks behaviour like this.

So I think changing: char *errortext = errorbuf ? errorbuf : buf; to

char *errortext = NULL
if errorbuf != NULL {
    errortext = errorbuf
}

Then in the code, only sprintf into errortext if != NULL

Does that seem reasonable?

Thanks. It was not the purpose of this fix but why not ?

I think it will make the code a little bit more complex. But we will be saving all the sprintf's each time we add a replica object.

errortxt buffer is used for both returning and logging an error msg. In case of error we need to log a message and have a buffer for that. I actually find the code quite elegant to toggle use of calling/stack buffer.
Creation of replica is quite rare so IMHO avoiding strcpy is not a big benefit.
I do agree that ternary code is not nice and prefer 'if-else'.
IMHO I think this part of the patch looks good (call/stack buffer and strcpy) but removal of ternary is good idea

Ternaries are really hard to read @gparente: I won't ack code that contains them.

People use ternaries thinking it makes their code "compact" or "faster" or "readable". None of this is true. Compact is the opposite of readable. It is really hard for people to parse and read, especially when people get "tricky" and think they can use multiple ternaies. It also masks and hides conditionally logic inside variable declarations. Finally, it is never faster, because C is not a low level language. Anything you write in C is going to be altered, rearranged and modified by a compiler to be "as fast as possible".

So with this in mind, it is always better to write a complete section of code, that is verbose, clear, and readable, and allow the compiler to do it's job

That's all :)

Hi William,

no problem. But this is not MY code. You don't need to to ack it if you don't want but I have never used a ternary in my code. It's not in my diff. If this is clear and you have understood it, I will remove the ternary in this PR as soon as possible.

Best regards :-)

@german, I think it is what @firstyear suggested. Taking the opportunity of your patch to do some additional cleanup (extra to the core of your patch) to make the function easier to understand.

Generally my personal rule is "if I'm already touching the function, testing it, and checking it" it's a net benefit to clean a little around what I'm doing too. :)

My personal rule is one ticket = one functionality. It's quite more clear to me to optimize code in a different ticket.

By the way, there are around ~1200 ternaries in the code to clean. Good luck.

@firstyear Hi, German is contributing a patch which resolves an issue and the patch itself seems to be good. I do not see a justification to reject it because some additional work could be done, especially if the contributer is not part of the dev team.

@firstyear I appreciate you will to cleanup the code each time it is possible.

Now regarding this specific ticket, @gparente accepted this ticket as a kind of training of dev process as he is not fully familiar with it and sometime spend a lot of time on things that may be obvious for others. Having @gparente as a regular contributor would be a big win for our dear 389-ds. So this ticket should rather be a welcome than a difficulty.

He already accepted to do an automatic testcase that I know is a new area for him. If code cleanup is an overhead I personally give the priority to the testcase.

@gparente, we may discuss the automatic test case next week. I know you will love the progress of lib389 !

I'm sorry if this came through as a rejection: I was trying to point out a possible issue in the patch, but it was not @gparente's fault - it caught me out as a reviewer due to an existing flawed piece of code. While it would be great for him to fix it, I'm not going to block the patch on this. Saying this, I also know if we don't clean-up today, we will forget and never come back to it ... I apologize for any angst this has caused, it wasn't the intent. I do fully agree and want @gparente to continue to contribute, as he is a very smart person and I hope to see much more code from him in the future :)

rebased onto 9e26e944fa6e5f9e65ad9cd5d8bca101de2fd46c

5 years ago

rebased onto 1612b2f8ee9e53a7fba217076e48bfcc3413acb9

5 years ago

rebased onto 0ec292667a115cad072d3da4674092365040f718

5 years ago

rebased onto 7f3f978

5 years ago

@firstyear , thanks ! I have applied the changes we have been discussing.

Thanks always for your remarks and review.

Regards,

German.

Looks great to me! ack :)

Thanks for the review.

Commit e08b20c fixes this pull-request

Pull-Request has been merged by gparente

5 years ago

Pull-Request has been merged by gparente

5 years ago

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/3138

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
Metadata