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

Fixes crash when deallocating rbt_node in windows #770

Merged
merged 1 commit into from
Jul 1, 2017
Merged

Fixes crash when deallocating rbt_node in windows #770

merged 1 commit into from
Jul 1, 2017

Conversation

WesleyCeraso
Copy link

Fixes OpenSCAP/scap-workbench#129 and OpenSCAP/scap-workbench#61

Signed-off-by: Wesley Ceraso Prudencio wcerasop@redhat.com

@yuumasato
Copy link
Member

Does the CI work for master branch?

Although the fix is intended for Windows, maybe we should proposed this to maint-1.2 branch.

@mpreisler
Copy link
Member

@yuumasato make check is broken on master AFAIK.

@WesleyCeraso
Copy link
Author

@yuumasato
We only build master for windows, as the guide says:
"Currently we have to use master branch to have Windows support"

If we move this fix to maint-1.2, shouldn't this "Windows support" be there as well?

@mpreisler
Copy link
Member

NACK.

Incomplete fix, the same issue is in the other free method. Instead of linking MSDN docs that anybody can google please explain why we need to do aligned_free. Ref where the aligned_malloc happens.

@mpreisler
Copy link
Member

Although the fix is intended for Windows, maybe we should proposed this to maint-1.2 branch.

maint-1.2 doesn't use aligned_malloc so this issue is not there. this should go to master only.

@WesleyCeraso
Copy link
Author

@mpreisler @yuumasato updated

// using free for memory allocated through _aligned_malloc is illegal
// rbt_str.c -> rbt_str_node_alloc
// https://msdn.microsoft.com/en-us/library/8z34s9c6.aspx
_aligned_free(rbt_walk_top());
Copy link
Member

Choose a reason for hiding this comment

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

wrong indents

// using free for memory allocated through _aligned_malloc is illegal
// rbt_i32.c -> rbt_i32_node_alloc
// https://msdn.microsoft.com/en-us/library/8z34s9c6.aspx
_aligned_free(rbt_node_ptr(n));
Copy link
Member

Choose a reason for hiding this comment

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

wrong indents

// using free for memory allocated through _aligned_malloc is illegal
// rbt_i64.c -> rbt_i64_node_alloc
// https://msdn.microsoft.com/en-us/library/8z34s9c6.aspx
_aligned_free(rbt_node_ptr(n));
Copy link
Member

Choose a reason for hiding this comment

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

wrong indents

Copy link
Author

Choose a reason for hiding this comment

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

@mpreisler I just followed the current identation, should I make a new identation in the file?

Copy link
Member

@mpreisler mpreisler Jun 30, 2017

Choose a reason for hiding this comment

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

we use tabs for indents, only reindent the code you changed to avoid merge conflicts

in other words all new code should follow the rules, keep the old code as it is

@WesleyCeraso
Copy link
Author

@mpreisler indentation fixed

@mpreisler
Copy link
Member

@WesleyCeraso rbt_free will still crash, won't it?

Also, could you use curly braces in case the statements are multi line? Somebody will at some point add another statement and the whole thing will stop working. It gets confusing especially with the macros.

Fixes OpenSCAP/scap-workbench#129 and OpenSCAP/scap-workbench#61

Signed-off-by: Wesley Ceraso Prudencio <wcerasop@redhat.com>
@mpreisler mpreisler added this to the 1.3.0 milestone Jul 1, 2017
@mpreisler mpreisler self-assigned this Jul 1, 2017
@mpreisler
Copy link
Member

ACK

@mpreisler mpreisler merged commit 1372f1c into OpenSCAP:master Jul 1, 2017
This was referenced Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants