stringop-truncation and address-of-packed-member #309
Conversation
…py with memcpy to fix errors related to stringop-truncation.
|
Hi NaruFGT! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| uint32_t *unpacked_identifiers = new uint32_t[sizeof(uint32_t)*keyfmt->num_identifiers]; | ||
| for(size_t i = 0; i < keyfmt->num_identifiers; ++i) { | ||
| unpacked_identifiers[i] = keyfmt->identifier_list[i]; | ||
| } | ||
| AttributeList attributes = AttributeList::Load(keyfmt->num_identifiers, unpacked_identifiers, rendition_key); |
There was a problem hiding this comment.
I think there's an alignment problem here.
| uint32_t *unpacked_identifiers = new uint32_t[sizeof(uint32_t)*keyfmt->num_identifiers]; | ||
| for(size_t i = 0; i < keyfmt->num_identifiers; ++i) { | ||
| unpacked_identifiers[i] = keyfmt->identifier_list[i]; | ||
| } | ||
| AttributeList attributes = AttributeList::Load(keyfmt->num_identifiers, unpacked_identifiers, rendition_key); |
| uint32_t *unpacked_identifiers = new uint32_t[sizeof(uint32_t)*keyfmt->num_identifiers]; | ||
| for(size_t i = 0; i < keyfmt->num_identifiers; ++i) { | ||
| unpacked_identifiers[i] = keyfmt->identifier_list[i]; | ||
| } | ||
| auto attributes_value = item.second.attributes().write(keyfmt->num_identifiers, unpacked_identifiers); |
| @@ -162,7 +162,11 @@ write() const | |||
| bom_tree_reserve(renditions_tree_context, rendition_count); | |||
| if (renditions_tree_context != NULL) { | |||
| for (auto const &item : _renditions) { | |||
| auto attributes_value = item.second.attributes().write(keyfmt->num_identifiers, keyfmt->identifier_list); | |||
| uint32_t *unpacked_identifiers = new uint32_t[sizeof(uint32_t)*keyfmt->num_identifiers]; | |||
There was a problem hiding this comment.
Memory leak + incorrect size specifier.
| @@ -264,7 +268,11 @@ lookupRenditions(Facet const &facet) const | |||
| KeyValuePair value = (KeyValuePair)it->second; | |||
| car_rendition_key *rendition_key = (car_rendition_key *)value.key; | |||
| struct car_rendition_value *rendition_value = (struct car_rendition_value *)value.value; | |||
| AttributeList attributes = AttributeList::Load(keyfmt->num_identifiers, keyfmt->identifier_list, rendition_key); | |||
| uint32_t *unpacked_identifiers = new uint32_t[sizeof(uint32_t)*keyfmt->num_identifiers]; | |||
There was a problem hiding this comment.
You're never freeing the memory block you allocated here. Also, the size you allocate seems wrong to me; you're gonna end up allocating 4 entries in your array (sizeof(uint32_t)).
|
Thank you for the feedback, I’m wondering if a better approach would be to add |
|
I think you are right that having a separate function to perform the
unpacking is a good idea. Also, looking at the code you're modifying, I'm
not sure that unpacking this stuff with a loop like you're doing is
necessary. You're dealing with arrays of `uint32_t` which would not expose
any alignment/packing issues on the architectures we are building on. Am I
missing something?
…On Tue, Dec 31, 2019 at 7:36 AM Parker Gibson ***@***.***> wrote:
Thank you for the feedback, I’m wondering if a better approach would be to
add
AttributeList AttributeList::
Load(size_t count, uint32_t const *identifiers, uint16_t const *values)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#309>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAMO3AT5KHQVIFUQWSXKWLQ3NRHRANCNFSM4KAKEI6Q>
.
--
Stephane Sezer
|
|
Rather than unpacking, I think the ideal solution is to pass keyfmt rather than passing the addresses of it's members. If we pass the address of keyfmt, we don't have to ever take the address of keyfmt->identifier_list which is the root cause of the build errors in the first place. |
|
I submitted #311 which preserves the behaviors in this PR while using std::vector which should call destructors once unpacked_identifiers is out of scope, also fixed formatting. I didn't realize my editor is using tabs rather than spaces. |
Fixed build errors related to passing address of packed
car_key_formatand replaced strncpy with memcpy to fix errors related to stringop-truncation.