Closed jianyan-li-qnx closed 4 months ago
Probably should have a new func in strings.c that returns a "NULL" string if the parameter is NULL.
Hi Eric,
Thanks for the quick reply. I have a quick question.
Probably should have a new func in strings.c that returns a "NULL" string if the parameter is NULL.
How is this different/better than using existing macro MU_P_OR_NULL
?
Because one would return a null value and the other is a string. That is why there are quotes around it.
MU_P_OR_NULL
returns a null string too, at deps/azure-macro-utils-c/inc/azure_macro_utils/macro_utils.h: 25
#define MU_P_OR_NULL(p) (((p)!=NULL)?(p):"NULL")
Am I missing something?
Thanks for the explanation. My original comment might be confusing too, let me rephrase it: I was talking about using MU_P_OR_NULL to encapsulate calls to STRING_c_str, which would result in something like this:
MU_P_OR_NULL(STRING_c_str(registered_device->device_id))
. This way, the null value returned by STRING_c_str can be converted to "NULL" and everything would work fine.
Your suggestion, as I understand it, is to create a new mockable function such that:
And then replace calls to STRING_c_str with calls to this new function
This obviously would work. But I can see that it might have a downside. The original STRING_c_str returns a null value which is easy to detect. Now with the new function you have to use strcmp to determine whether the return value of the function represents a null value or not. Moreover, this new function might cause confusion in the edge case where the string handle's string is "NULL" to begin with.
With a new STRING_c_str you can be more explicate what is NULL. Something like this
const char* STRING_c_str_null_string(STRING_HANDLE handle)
{
const char* result;
if (handle != NULL)
{
result = ((STRING*)handle)->s;
if (result == NULL)
{
result = "<NULL VALUE>";
}
}
else
{
result = "<NULL HANDLE>";
}
return result;
}
Moreover, this new function might cause confusion in the edge case where the string handle's string is "NULL" to begin with.
This would more be opt-in than opt-out, so you should know the use case, which should be limited to logging.
I see, if this function is for logging only then it makes sense to do that and it would solve the problem we have with STRING_c_str
.
However, there are some other cases where doing this does not solve the problem. One somewhat frequent problem is that existing code tries to print a null char* instead of a string handle. There are few examples of this, such as https://github.com/Azure/azure-iot-sdk-c/blob/main/serializer/src/schema.c#L2446, https://github.com/Azure/azure-iot-sdk-c/blob/main/serializer/src/iotdevice.c#L305, and https://github.com/Azure/azure-iot-sdk-c/blob/main/iothub_client/src/iothub_client_core.c#L2385
Since these cases print a char instead of a string handle, STRING_c_str_null_string
does not work. Perhaps I should use MU_P_OR_NULL
to encapsulate the char to solve this problem?
@jianyan-li-qnx please validate this branch.
@jianyan-li-qnx please validate this branch.
2594
@ericwol-msft Thanks for the response.
The changes you proposed works on my end.
However I have to also change deps/azure-ctest/src/ctest.c:288
to this to make it fully working, because the test runner is also trying to print NULL
:
void char_ptr_ToString(char* string, size_t bufferSize, const char* val)
{
(void)snprintf(string, bufferSize, "%s", MU_P_OR_NULL(val));
}
void wchar_ptr_ToString(char* string, size_t bufferSize, const wchar_t* val)
{
(void)snprintf(string, bufferSize, "%ls", MU_WP_OR_NULL(val));
}
What is the call stack? We can't change anything in the deps/azure-ctest repo.
What is the call stack? We can't change anything in the deps/azure-ctest repo.
CTEST_ASSERT_ARE_EQUAL is defined as CTEST_ASSERT_ARE_EQUAL, which then calls the _ToString functions
iothub_client/tests/iothub_client_properties_ut/iothub_client_properties_ut.c:1293 ASSERT_ARE_EQUAL(char_ptr, expectedProperty->componentName, actualProperty->componentName); Both componentNames here are null
iothub_client/tests/iothub_client_properties_ut/iothub_client_properties_ut.c:1326 CompareProperties
iothub_client/tests/iothub_client_properties_ut/iothub_client_properties_ut.c:1487 IoTHubClient_Properties_Deserializer_GetNext_three_reported_update_properties => TestDeserializedProperties
Fixed in PR #2594
Hi team!
My name is Jianyan and I've been working on porting azure-iot-sdk-c LTS_08_2023 to QNX.
I noticed this problem where some test programs would seg fault. Upon further investigation, this is what I think happened:
Take
Transport_Fully_Reconnects_After_5_AMQP_device_errors
iniothub_client/tests/iothubtransport_amqp_common_ut/iothubtransport_amqp_common_ut.c
, in LTS_08_2023, as an example:STRING_c_str
without specifying return value of the mock function, atiothubtransport_amqp_common_ut.c:4465
IoTHubTransport_AMQP_Common_DoWork
atiothubtransport_amqp_common_ut.c:4469
IoTHubTransport_AMQP_Common_Device_DoWork
is called atiothubtransport_amqp_common.c:1470
LogError
is called atiothubtransport_amqp_common.c:1071
with the argumentSTRING_c_str(registered_device->device_id)
as a%s
STRING_c_str
returnsNULL
, so LogError is called withNULL
as the%s
LogError
invokesconsolelogger_log
, which invokesvprintf
withNULL
as a%s
.Please see the full stacktrace below:
Please confirm, if possible, that my conclusion is correct.
I am not sure what is the most idiomatic way of solving this issue. I am thinking of sanitizing the string with
MU_P_OR_NULL
before each call toLogError
.