Issue 6453 - Fix memory leaks in entryrdn #6455
Conversation
| @@ -1570,12 +1576,15 @@ _entryrdn_get_elem(entryrdn_db_ctx_t *ctx, | |||
| key->data, data->size, data->ulen, rc); | |||
| } | |||
| _ENTRYRDN_DEBUG_GOTO_BAIL(); | |||
| slapi_ch_free((void**)elem); | |||
There was a problem hiding this comment.
This will prevent the log of the rdn with the error
slapi_log_err(SLAPI_LOG_TRACE, "_entryrdn_get_elem", "<-- _entryrdn_get_elem (elem rdn=%s) rc=%d\n",
RDN_ADDR(elem), rc);
IMHO we should rather call if (rc) { slapi_ch_free((void)elem); } after login the error in the bail section.
| @@ -1695,12 +1711,18 @@ _entryrdn_get_tombstone_elem(entryrdn_db_ctx_t *ctx, | |||
| if (0 == strcmp(comma + 1, slapi_rdn_get_nrdn(srdn))) { | |||
| /* found and done */ | |||
| _entryrdn_dup_rdn_elem((const void *)dataret.data, elem); | |||
| if (RDN_IS_REDIRECT(childelem)) { | |||
| slapi_ch_free((void **)&childelem); | |||
There was a problem hiding this comment.
this sounds suspicious because childelem is an array elems ...
There was a problem hiding this comment.
this sounds suspicious because childelem is an array elems ...
It fixes the leak. Note childelem and dataret.data point to the same address. I just chose to use childelem.
| rc = dblayer_cursor_op(&ctx->cursor, dbop, key, data); | ||
| elem_data = data->data; /* save pointer to data so we can free it on a retry */ |
There was a problem hiding this comment.
would not be slapi_ch_free((void**)elem); sufficient ?
There was a problem hiding this comment.
No because elem ptr is changed after we grab the original ptr address (when a retry occurs for a long rdn). So we need to free the previous value (elem_data). I'm reworking the variable name to be "prev_elem_data" so it's more clear.
There was a problem hiding this comment.
TBH this was a really tricky set of leaks to fix. Took me a long time to finalize the fix.
Description: We leak memory when processing long DN's (mdb). We have to loop over long DN's and on each pass we leak the previous rdn element. In the tombstone case we just need to free the current childelem Relates: //github.com/389ds/issues/6453 Reviewed by: progier(Thanks!)
mreynolds389
force-pushed
the
entryrdn_leaks
branch
from
100ec32 to
c7801b7
Compare
| if (*elem) { | ||
| /* free the previous elem data */ | ||
| slapi_ch_free((void**)&prev_elem_data); | ||
| } |
There was a problem hiding this comment.
I wonder if we also need to check and free it at the bail: section
There was a problem hiding this comment.
I wonder if we also need to check and free it at the
bail:section
Maybe :-) Like I said this code is a bit tricky when it comes to handling long DN's vs not long DN's. I'll try and force some failures and see what happens...
Description:
We leak memory when processing long DN's (mdb). We have to loop over long DN's and on each pass we leak the previous rdn element. In the tombstone case we just need to free the current childelem
Relates: #6453
Reviewed by: ?