PS-9673 fix of bug with double memory freeing in case of an error

[lukin-oleksiy]

https://perconadev.atlassian.net/browse/PS-9673

The problem: encoding buffer address is referenced in 2 places, in the local variable and in the ctx->buffer. When buffer was dealocated with "kmip_free_buffer(&ctx, encoding, buffer_total_size)" call, and then kmip_destroy(&ctx) was called, we had double deallocation. To fix the problem, "kmip_set_buffer(&ctx, NULL, 0)" call was added before context destruction call in all places. There's no such problem in "happy path" because "kmip_set_buffer(&ctx, NULL, 0)" is there.

This is not the ideal solution, because whole "kmip_bio.c" is a messy code that should be re-written ASAP.

[satya-bodapati]

@lukin-oleksiy also interestd to know what is the error here (from Mohit's script)? and does check_keyring() handle errors?

[mohitj1988]

The memory leak issue is fixed. With the latest PR changes, the crash is not seen. Approved the PR

[dutow]

Wouldn't it be simpler to modify kmip_free_buffer with ctx->buffer = ctx->index = NULL; ctx->size = 0;?

That makes sense there, and should fix the issue. On the other hand, a todo comment in kip_set_buffer suggests the original developers wanted to add NULL handling there, which would make this patch incorrect.

[lukin-oleksiy]

Wouldn't it be simpler to modify kmip_free_buffer with ctx->buffer = ctx->index = NULL; ctx->size = 0;?

That makes sense there, and should fix the issue. On the other hand, a todo comment in kip_set_buffer suggests the original developers wanted to add NULL handling there, which would make this patch incorrect.

kmip_free_buffer() is used in other places and it is questionable to handle or reference "ctx->buffer" here. It could be anything, in arguments of kmip_free_buffer(), not just double-referenced "ctx->buffer" like in the context of changed code. The code should use just one variable to reference buffer memory, and in this context is should be "ctx->buffer". But it is not. And this is an classical example of bad code.

Generally speaking, the "kmip_ bio" module should be replaced with clean and well-designed code. That's what Mondo team already did.

[lukin-oleksiy]

@lukin-oleksiy also interestd to know what is the error here (from Mohit's script)? and does check_keyring() handle errors?

Satia! The error is attempt to free already freed chunk of memory. And such kind of errors, as you know, can not be handled in "normal' way.

[satya-bodapati]

@lukin-oleksiy also interestd to know what is the error here (from Mohit's script)? and does check_keyring() handle errors?

Satia! The error is attempt to free already freed chunk of memory. And such kind of errors, as you know, can not be handled in "normal' way.

@lukin-oleksiy, I mean, double free is in the error code path. I would like to know the KMIP error that occurred and why it occurred on Encryption::check_keyring() calls from PS?

[mohitj1988]

As suggested by @satya-bodapati , I did the same tests to repeat this problem in PS-8.0.37 and PS-8.0.39 (Note there was no 8.0.38 released). This issue is clearly a regression introduced in PS 8.0.39 as the problem does not exists in PS 8.0.37

Possible commit that is causing it:

`commit b1135408b1d1585112871bfbf870bd6cebf3753f
Author: Oleksiy Lukin [email protected]
Date: Wed Sep 4 15:55:39 2024 +0300

PS-9286 KMIP key activation implementation

https://perconadev.atlassian.net/browse/PS-9286

extra/libkmip head is at baf6832 with activate operation implementation.
keyring_kmip/backend/backend.cc updateed to execute op_activate after key
registration.`

[lukin-oleksiy]

@satya-bodapati The situation you described in not related to this patch. It happens because of key delete operation failure. I implemented "activate" operation earlier as solution for previous bug (PS-9286). But we forgot to implement "revoke" operation to deactivate key. And then we discovered that active key can not be deleted. That's why we have set of problems, including one you mentioned above. I'm working on another patch that implements "revoke" and will publish it soon.

[satya-bodapati]

@satya-bodapati The situation you described in not related to this patch. It happens because of key delete operation failure. I implemented "activate" operation earlier as solution for previous bug (PS-9286). But we forgot to implement "revoke" operation to deactivate key. And then we discovered that active key can not be deleted. That's why we have set of problems, including one you mentioned above. I'm working on another patch that implements "revoke" and will publish it soon.
Thanks for the explanation @lukin-oleksiy

[satya-bodapati]

@lukin-oleksiy imo, we should also mention the reason for error/error path in your commit message.