Page MenuHomePhabricator

Fix memory leaks of buffer allocated in __kmp_str_format()

Authored by vhscampos on Nov 16 2016, 6:45 PM.



Following comment in kmp_str.c line 480, strings allocated by kmp_str_format() have to be freed using kmp_str_free(). In many instances, these buffers were freed by calling free() directly.

In almost all calls to kmp_msg(), KMP_ERR and KMP_SYSERRCODE are used to allocate messages. This allocation uses kmp_str_format() as well, but the buffer is not freed afterwards. I made changes aiming to eliminate these leaks.

Event Timeline

vhscampos updated this revision to Diff 78303.Nov 16 2016, 6:45 PM
vhscampos retitled this revision from to Fix memory leaks of buffer allocated in __kmp_str_format().
vhscampos updated this object.
vhscampos added a subscriber: openmp-commits.
AndreyChurbanov requested changes to this revision.Nov 17 2016, 6:17 AM
AndreyChurbanov edited edge metadata.
  1. __kmp_msg(kmp_ms_fatal,...) never returns, and it cleans up its arguments inside. Thus trying to clean its argument once more time is an error.
  2. __kmp_msg(kmp_ms_warning,...) cleans its arguments in case it tries to print anything, and doesn't clean in case it returns immediately. Thus the added cleanup action should be conditional, e.g.
if(__kmp_generate_warnings == kmp_warnings_off)

I tried to mark all changes those need to be under condition, and those are OK. All the changes for fatal messages should be reverted.

61 ↗(On Diff #78303)

This change is bad, as well as all other changes for __kmp_msg(kmp_ms_fatal,...).

569 ↗(On Diff #78750)

This change is OK, as the __kmp_str_free frees and nullifies parameter.

144 ↗(On Diff #78750)

Needs condition.

433 ↗(On Diff #78750)

Needs condition.


OK to remove the comment.

842 ↗(On Diff #78750)

I think keeping all declarations together looks better that moving one here. Why this change?

944 ↗(On Diff #78750)

These two changes are OK.

970 ↗(On Diff #78750)

These two are OK as well.

107 ↗(On Diff #78750)

Needs condition.

107 ↗(On Diff #78750)

Needs condition.

143 ↗(On Diff #78750)

Needs condition.

152 ↗(On Diff #78750)

Needs condition.

363 ↗(On Diff #78750)

This is OK.

421 ↗(On Diff #78750)

These two are OK.

2238 ↗(On Diff #78750)

This is OK.

423 ↗(On Diff #78750)

This is OK.



175 ↗(On Diff #78750)

Needs condition.

211 ↗(On Diff #78750)

Needs condition.

263 ↗(On Diff #78750)

Needs condition.

298 ↗(On Diff #78750)

Needs condition.

737 ↗(On Diff #78750)

Needs condition.

984 ↗(On Diff #78750)

Needs condition.

1095 ↗(On Diff #78750)

Needs condition.

1155 ↗(On Diff #78750)

Needs condition.

570 ↗(On Diff #78750)

Needs condition.

This revision now requires changes to proceed.Nov 17 2016, 6:17 AM
vhscampos marked 25 inline comments as done.Nov 17 2016, 11:11 AM

Ok, I'm fixing the patch. Thanks.

vhscampos updated this revision to Diff 78395.Nov 17 2016, 11:14 AM
vhscampos edited edge metadata.

Applied your comments.

AndreyChurbanov requested changes to this revision.Nov 21 2016, 8:00 AM
AndreyChurbanov edited edge metadata.
AndreyChurbanov added inline comments.
143–144 ↗(On Diff #78750)

Something is wrong with indents here. Please check that replacement of tabs with spaces done correctly in this code block.

428 ↗(On Diff #78750)

It should be err_msg.str variable, not err_code.str. Or, alternatively, name it err_code in lines 419, 423 and here.

This revision now requires changes to proceed.Nov 21 2016, 8:00 AM
vhscampos updated this revision to Diff 78750.Nov 21 2016, 12:03 PM
vhscampos edited edge metadata.
vhscampos marked 2 inline comments as done.

Fixed indentation and variable name issues.

AndreyChurbanov accepted this revision.Nov 22 2016, 1:26 AM
AndreyChurbanov edited edge metadata.


This revision is now accepted and ready to land.Nov 22 2016, 1:26 AM

shell I commit the change on your behalf?

This revision was automatically updated to reflect the committed changes.