wh_server: fix server comm not initialized from config #235
Conversation
wh_Server_Init never assigns server->comm from config->comm before calling wh_CommServer_Init(server->comm, ...). That passes an uninitialized (zeroed) pointer to wh_CommServer_Init and will likely crash or behave incorrectly. Suggested minimal fix Assign server->comm = config->comm (and validate config->comm != NULL) before calling wh_CommServer_Init. Signed-off-by: Badr Bacem KAABIA <badrbacemkaabia@gmail.com>
There was a problem hiding this comment.
Hi @kaabia
Thanks for contributing to wolfHSM.
I'm a bit confused what you are trying to do here? Your code is using struct fields that don't exist, and seems to be conflating transport layer configuration with higher level server configuration.
Also, please ensure that your code builds locally before submitting a PR.
| if (config->comm == NULL) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
| server->comm = config->comm; |
There was a problem hiding this comment.
config->comm doesn't exist (which is why you are failing in CI).
What are you trying to do here? The transport-specific context is meant to be opaque and only used by the various transport back-ends, and so should be NULL checked at that level.
There was a problem hiding this comment.
You are absolutely right. I apologize for the confusion and the incorrect submission. I had misunderstood the initialization flow.
As you pointed out, my change was attempting to access config->comm, which does not exist. The correct member is config->comm_config.
More importantly, I now understand that a direct assignment is not the correct approach. The wh_CommServer_Init() function is called later in wh_Server_Init() and is responsible for initializing the whCommServer structure within the server context. It takes server->comm and config->comm_config as arguments and handles the necessary setup, including the NULL check for the configuration.
I was basically trying to add lines like:
context->transport_context = config->transport_context;
context->transport_cb = config->transport_cb;
context->server_id = config->server_id;And, classic dump fault, I forgot to add them before pushing the PR. However, upon reviewing the code, I now realize these assignments are already correctly handled within the wh_CommClient_Init function
Thank you for your feedback. I will be closing this pull request.
|
My proposed change was redundant and fundamentally incorrect. |
3 participants
This PR addresses an issue during server initialization (
wh_Server_Init) by adding a null pointer check for theconfig->comm(communication context) parameter. It also explicitly assigns thecommcontext to theservercontext immediately after the check.