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

Ameba: Mismatching allocation and deallocation #4853

Closed
wants to merge 1 commit into from

Conversation

janjongboom
Copy link
Contributor

Not sure if this is actually a problem when running, but syntactically it's not correct.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 3, 2017

/morph test

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 3, 2017

cc @Archcady

@@ -75,7 +75,7 @@ static rtw_result_t scan_result_handler( rtw_scan_handler_result_t* malloced_sca
ap.channel = record->channel;
WiFiAccessPoint *accesspoint = new WiFiAccessPoint(ap);
memcpy(&scan_handler->ap_details[scan_handler->ap_num], accesspoint, sizeof(WiFiAccessPoint));
delete[] accesspoint;
delete accesspoint;
}
scan_handler->ap_num++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Archcady Looking at the code around, got few questions, why this allocation is needed at first place, why ap_num is incremented also if the condition above is false (thus no memcpy is done) ? That memcpy is done on non POD data , should not.

@pan- Thanks for pointers

Copy link
Contributor

@Archcady Archcady Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @0xc0170 I think in this part we use allocation is to borrow the data structure of this class so we might use this scan_handler->ap_details[] somewhere else as a pointer to an instance of WiFiAccessPoint.
line 80 scan_handler->ap_num__; seems like being placed here on purpose, scan_handler->ap_details[scan_handler->ap_num] has reserved its place for accesspoint data, so if the condition above is false, the space is left zero and ap_num++ to start process next accesspoint data.
Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Archcady Better use placement new then. The behavior of the program is undefined (from the C++ standard standpoint) if a non POD object is initialized with memcpy.

You can replace:

WiFiAccessPoint *accesspoint = new WiFiAccessPoint(ap);
memcpy(&scan_handler->ap_details[scan_handler->ap_num], accesspoint, sizeof(WiFiAccessPoint));
delete accesspoint;

by

new(&scan_handler->ap_details[scan_handler->ap_num]) WiFiAccessPoint(ap);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pan- , BTW do I need to call destructor explicitly here? I'm not familiar with placement new in C++.

Copy link
Member

@pan- pan- Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Archcady Placement new does not allocate memory from the heap. It use the address supplied in the placement param to construct the object.

In this specific case, the object will be constructed at the address &scan_handler->ap_details[scan_handler->ap_num]. You'll have to call directly the destructor when the ap_details memory block goes away or the delete[] operator if ap_details have been allocated with new[] (which I doubt since the data structure is defined locally).

@mbed-bot
Copy link

mbed-bot commented Aug 4, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 933

All builds and test passed!

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 4, 2017

@Archcady Will you send separate PR to address the allocation discussed here? To know how to proceed with this fix. I would keep this open until all concerns are addressed, and then this can be closed?

@Archcady
Copy link
Contributor

Archcady commented Aug 7, 2017

Thanks @pan- , @0xc0170 , I send a PR to solve this. Please see #4863 .

@theotherjimmy
Copy link
Contributor

@janjongboom Closing as this has been superseded by #4863.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants