Skip to content

Issue 6555 - Potential crash when deleting a replicated backend - #6559

Merged
progier389 merged 1 commit into
389ds:mainfrom
progier389:i6555
Jan 30, 2025
Merged

progier389 merged 1 commit into
389ds:mainfrom
progier389:i6555

Conversation

@progier389

@progier389 progier389 commented Jan 29, 2025 •

Copy link
Copy Markdown
Contributor

Problem: connection cleanup may try to access a freed replica
Solution: Ensure that the replica is still existing before using its pointer

Issue: #6555

Reviewed by: @tbordaz, @droideck (Thanks!)

@tbordaz tbordaz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@droideck droideck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good!

void replica_set_updatedn(Replica *r, const Slapi_ValueSet *vs, int mod_op);
void replica_set_groupdn(Replica *r, const Slapi_ValueSet *vs, int mod_op);
char *replica_get_generation(const Replica *r);
bool replica_check_validity(Replica *replica);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit inconsistent to use bool while every other line in the file uses PRBool... But I understand we might get rid of PRBool at some point.

I don't have a hard opinion here, but generally, I'd prefer consistency in this case (my 5 cents). :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I discussed that one with Mark in another review, and finally we decided to use stdbool in new code.
I agree it is inconsistent, until we do a global cleanup phase (but frankly I am afraid that there are dependencies toward the plugin API and we will never be able to complete that move )

@progier389
progier389 merged commit f5e5b59 into 389ds:main Jan 30, 2025
progier389 added a commit to progier389/389-ds-base that referenced this pull request Feb 5, 2025
…nd (389ds#6559)"

This reverts commit f5e5b59.

because with this commit server may hang at shutdown
progier389 added a commit to progier389/389-ds-base that referenced this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants