#50492 Ticket 50459 - Correct issue with allocation state
Closed 3 years ago by spichugi. Opened 4 years ago by firstyear.
firstyear/389-ds-base 50459-c_state-invalid  into  master

@@ -117,6 +117,8 @@ 

      Connection *c = NULL;

      int index, count;

  

+     PR_Lock(ct->table_mutex);

+ 

      index = sd % ct->size;

      for (count = 0; count < ct->size; count++, index = (index + 1) % ct->size) {

          /* Do not use slot 0, slot 0 is head of the list of active connections */
@@ -138,7 +140,6 @@ 

          PR_ASSERT(c->c_extension == NULL);

  

          if (c->c_state == CONN_STATE_FREE) {

-             PR_Lock(ct->table_mutex);

  

              c->c_state = CONN_STATE_INIT;

  
@@ -151,15 +152,11 @@ 

              }

  

              c->c_pdumutex = PR_NewLock();

-             PR_Unlock(ct->table_mutex);

              if (c->c_pdumutex == NULL) {

                  c->c_pdumutex = NULL;

                  slapi_log_err(SLAPI_LOG_ERR, "connection_table_get_connection", "PR_NewLock failed\n");

                  exit(1);

              }

-         } else {

-             slapi_log_err(SLAPI_LOG_ERR, "connection_table_get_connection", "Invalide connection table state - We tried to allocate to a conn NOT in state CONN_STATE_FREE - this is a complete disaster!\n");

-             exit(1);

          }

          /* Let's make sure there's no cruft left on there from the last time this connection was used. */

          /* Note: no need to lock c->c_mutex because this function is only
@@ -173,6 +170,9 @@ 

          /* couldn't find a Connection */

          slapi_log_err(SLAPI_LOG_CONNS, "connection_table_get_connection", "Max open connections reached\n");

      }

+ 

+     PR_Unlock(ct->table_mutex);

+ 

      return c;

  }

  

Bug Description: While adding the connection state, due to
a misunderstanding on my part, it was possible that a connection
was more likely to fail to allocate causing the server to exit(1)
incorrectly.

Fix Description: Fix the state handler to correctly account for
connection structure reuse.

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

Author: William Brown william@blackhats.net.au

Review by: ???

@firstyear, I agree with the fix but disagree with to make it on top of https://pagure.io/389-ds-base/pull-request/50485

#50459 and PR 50485 are to change c_mutex into pthread_mutex

The current PR https://pagure.io/389-ds-base/pull-request/50492# is related to #50489 that I opened yesterday. This is an independent problem from #50459. It is about make connection_table_get_connection thread safe.

I link https://pagure.io/389-ds-base/pull-request/50492# with https://pagure.io/389-ds-base/issue/50489.
As I said the fix looks good. Please rebase it on master not top of pending #50459 .

@tbordaz 50459 was merged to master? So this was based off from master actually, not from 50459. So maybe I misunderstand your concern here, because this fix is directly related to the code I merged to master, and it has a potential issue in it.

@firstyear, Sorry my mistake. I missed that you already merged https://pagure.io/389-ds-base/pull-request/50485 (#50459) into master.

We have #50459 that is to move c_mutex from NSPR to pthread_lock. It is fixed with https://pagure.io/389-ds-base/pull-request/50485.

I do not like the idea to have a second PR for #50459. This second PR is to make connection_table_get_connection thread safe that is #50489. #50489 should not be closed as duplicate as #50459 as it is a different problem. Also it is error prone while if we need to frontport or backout a fix.

The patch is valid and you have my ACK. Could you update this PR so that is only linked to #50489.

This patch isn't to address a nunc-stans issue like in #50489 - it's directly a bug as a result of the work in #50459. That's why I linked it as such.

You have acked though, so I'll merge, because it's going to be annoyig if people hit this in development.

rebased onto d5b23dc

4 years ago

Pull-Request has been merged by firstyear

4 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/3549

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