Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: CBOR Key/Value Arrays are interpreted correctly but do not deallocate correctly. #194

Closed
kbranner opened this issue Aug 17, 2021 · 7 comments
Assignees
Labels

Comments

@kbranner
Copy link

The JSON object shown below when translated to CBOR will be interpreted correctly , but the array key/value pair will never be de allocated using the cbor_decref call.
It turns out that for some reason the refcount for those items are set to a value of 2. So the cbor_decref(cbor_item_t **item_ref) function ignores refcounts > 1.
The reason why this occurs is because within the cbor_array_push function the cbor_incref(pushee) call will increment an item's refcount from 1 to 2.
For those items within the array they never get released by cbor_decref because of the requirement that their refcount must be equal to 1.

{
"state":
{
"desired":
{
"array" : [360,0,0,0,0,0,0,0]
}
}
}

@kbranner kbranner changed the title CBOR Key/Value Arrays are interpreted correctly but do not deallocate correctly. BUG: CBOR Key/Value Arrays are interpreted correctly but do not deallocate correctly. Aug 17, 2021
@kbranner
Copy link
Author

Here is the solution that I have currently implemented for the problem. It would be nice if the developers review this fix within cbor_decref:
case CBOR_TYPE_ARRAY: {
/* Get all items and decref them */
cbor_item_t **handle = cbor_array_handle(item);
size_t size = cbor_array_size(item);
for (size_t i = 0; i < size; i++)
{
if (handle[i] != NULL)
{
//================================================================
//NOTE : THe following code was modified my Kirk Branner. Hoping that the
// LibCbor developers fix this correctly. This is a best guess fix!
// Basically the reference count is set to 2 because both the
// original item and the array element is both referencing the value.
// So declare it here that the array is no longer referencing the value
// by decrementing the amount by 1!
cbor_item_t *checkItem = handle[i];
if (checkItem->refcount > 1)
{
checkItem->refcount--;
}
//========================Back to original code===================
cbor_decref(&handle[i]);
}
}
_CBOR_FREE(item->data);
break;
}

@kbranner kbranner reopened this Aug 18, 2021
@PJK PJK self-assigned this Aug 19, 2021
@PJK PJK added the bug label Aug 19, 2021
@PJK
Copy link
Owner

PJK commented Aug 19, 2021

Hi @kbranner , thank you for the bug report, could you please provide a full code example to reproduce the bug?

cbor_array_push incrementing the reference count seems to WAI on the surface because both the array and the caller will hold a reference to the item. You can cbor_move the item if the caller wants to release the reference.

@kbranner
Copy link
Author

Hi PJK.
Yes - I discovered that the array and caller are both referencing the array! Is there anything wrong with my fix?

I don't want to use cbor_move but simply interpret the full CBOR object and then de allocate it with a cbor_decref from the root. These objects CBOR objects in our application can get quite large - for example 65K bytes so trying to do cbor moves will add another layer of complexity.
Perhaps below is a better example and is valid JSON code. It is defining an array of 7 items, one for each day of the week, and gets interpreted by the embedded product to store the information for customer's use. So a customer could now display this week's temperatures once this information is received. The embedded product would parse the "weeks_temperatures" string and determined that the array contains temperatures for the week.
{
"state":
{
"desired":
{
"weeks_temperatures" : [95,90,89,88,90,90,88]
}
}
}

@PJK
Copy link
Owner

PJK commented Aug 21, 2021

Just to be clear, cbor_move doesn't actually do any memory allocation or copying, it simply decrements the reference count without deallocating the object with the assumption that the callee is taking ownership of it, so I think it does exactly what you want (release the handle in the current scope and only keep the pushed objects in the array)

@kbranner
Copy link
Author

Hi PJK -

Ok will give it a try and will report results back to you.

@kbranner
Copy link
Author

Hi PJK
Yes the cbor_move call for the array element works and solves the problem without any modifications to libcbor library.
Couple suggestions -
Suggestion #1: I would suggest that this issue be documented in your library documentation. I was under the impression that using cbor_decref from the root element will de-allocate all elements in the tree. But this is not true for array elements.

Suggestions#2: Perhaps you could introduce a new command such as cbor_deallocate(cbor_item_t **item_ref). This command will automatically de-allocate all elements from parameter item that was passed without the user requiring intimate knowledge details of the libcbor library.

Also thanks for this library and your continued support.
Please close this case
Kirk

@PJK
Copy link
Owner

PJK commented Aug 25, 2021

Glad to hear!

Re 1: Makes sense, filed #195

Re 2: Doesn't cbor_decref do what you want? You can find more at https://libcbor.readthedocs.io/en/latest/api/item_reference_counting.html

@PJK PJK closed this as completed Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants