Greetings All,
I am still digging into this trying to access the impact, but I believe it is
significant enough of a problem, especially given the pending 1.3 release, to
bring it up at this point. This message might be a bit long, but it is
important I think:
As pointed out by Alex Kelly, the following macro, is defined in ocpayload.h
#define VERIFY_CBOR_SUCCESS(log_tag, err, log_message) \
if ((CborNoError != (err)) && (CborErrorOutOfMemory != (err))) \
{ \
if ((log_tag) && (log_message)) \
{ \
OIC_LOG_V(ERROR, (log_tag), "%s with cbor error: \'%s\'.", \
(log_message), (cbor_error_string(err))); \
} \
goto exit; \
} \
Notice that CborErrorOutOfMemory is not considered an error condition. Looks
like this was written with the intent to follow that with an out of memory
check,
VERIFY_CBOR_SUCCESS(TAG, cborEncoderResult, "Failed adding roles name tag");
VERIFY_CBOR_NOT_OUTOFMEMORY(TAG, cborEncoderResult, "Not enough memory for
roles name tag")
However, there is only one function in all of IoTivity that actually does that
subsequent memory check. Of the about 800 locations where VERIFY_CBOR_SUCCESS
is used, only about 40 are followed by VERIFY_CBOR_NOT_OUTOFMEMORY, which means
in about 760 places in the code, out of memory is not being detected or
responded to. Seems like a pretty big deal.
Next I created this macro,
#define VERIFY_CBOR_NO_ERROR(log_tag, err, log_message) \
if ( CborNoError != (err) ) \
{ \
if ((log_tag) && (log_message)) \
{ \
OIC_LOG_V(ERROR, (log_tag), "%s with cbor error: \'%s\'.", \
(log_message), (cbor_error_string(err))); \
} \
if (CborErrorOutOfMemory == (err)) \
{ \
OIC_LOG_V(ERROR, "CBOR_NO_MEM", "Ran out of memery at
file/func/line: %s, %s, %d", \
__FILE__, __func__, __LINE__ ); \
} \
goto exit; \
} \
… which does not bypass CborErrorOutOfMemory, and reports when an out of memory
condition is encountered. I then used this macro, instead of the original, for
all ~760 instances that code is not checking for out of memory error.
Then, after that I started running unit tests to see what the impact of
actually detecting out of memory conditions would be. It has been kind of slow
going, but so far I have found that rdtests hits the out of memory condition
about 2 million times (literally), at these locations
resource/csdk/stack/src/ocpayloadconvert.c, OCConvertArray, 808
resource/csdk/stack/src/ocpayloadconvert.c, OCConvertRepMap, 844
resource/csdk/stack/src/ocpayloadconvert.c, OCConvertRepPayload, 969
resource/csdk/stack/src/ocpayloadconvert.c, OCConvertSingleRepPayload, 931
resource/csdk/stack/src/ocpayloadconvert.c, OCConvertSingleRepPayload, 934
I am now running resource/unittest (it is taking forever), but it is hitting
the out of memory error lots more than rdtests
So, the code as it currently stands essentially has no protection against CBOR
memory exhaustion in about about 80% of the encoding cases, and we are hitting
that conditions quite frequently in some of the unit tests.
Is this a release stopper?
Kind Regards
Steve
CableLabs
PS: If you want to follow the entire conversation you can see it here:
https://jira.iotivity.org/browse/IOT-2728
_______________________________________________
iotivity-dev mailing list
iotivity-dev@lists.iotivity.org
https://lists.iotivity.org/mailman/listinfo/iotivity-dev
