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 for arm64 / aarch64 #3

Closed
wants to merge 2 commits into from
Closed

Conversation

mkurz
Copy link

@mkurz mkurz commented Mar 23, 2023

Fixes #2

@@ -99,7 +99,6 @@ size_t _BDATA::grow( size_t new_real )
if( data_buff != NULL )
{
memcpy( new_buff, data_buff, data_real );
delete [] data_buff;
Copy link
Author

Choose a reason for hiding this comment

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

The double free or corruption error occurs when memory is freed twice or when memory is freed that was not previously allocated. It seems that the data_buff pointer is being freed twice: once in the grow method and again in the ~_BDATA destructor.

To fix that we can just don't try to free the memory in the grow method.

Choose a reason for hiding this comment

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

I think your changes would hide an already-existing use-after-free bug. If "data_buff" was already freed, and we just copied data out of it, then what the heck did we just copy?

I'm working on my own fix right now. The root cause in my test case (missing configuration data) appears to be in _CONFIG_MANAGER::file_vpn_load. When config.get_ispublic() is true, if sites_all isn't set, then it adds an empty string to the path. That alone isn't a problem, but then it goes to try to add a delimiter to "size-1", which causes an integer overflow. It doesn't appear ins was written to detect this edge case.

@@ -405,7 +405,7 @@ bool _CONFIG_MANAGER::file_vpn_del( CONFIG & config )

bool read_line_pcf( FILE * fp, BDATA & name, BDATA & data )
{
char next;
int next;
Copy link
Author

Choose a reason for hiding this comment

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

We should not compare unsigned chars to EOF (-1), so here the fix is to avoid casting the return value of fgetc, otherwise the config will not be loaded correctly (the casted value will be 255 instead of -1)

The exactly same problem happened here: https://bugs.webkit.org/show_bug.cgi?id=144439

@mkurz
Copy link
Author

mkurz commented Mar 23, 2023

Closing. First commit (fix "double free or corruption" on aarch64) is superseded by this patch: https://aur.archlinux.org/cgit/aur.git/tree/fix_double_free.patch?h=ike&id=d5d2736477cfc1df9b17e946a308e633bb8e0d6a

@mkurz mkurz closed this Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

double free or corruption (out) Aborted (core dumped)
2 participants