After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 777787 - Heap corruption due to wrong free
Heap corruption due to wrong free
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: nm-connection-editor
1.6.x
Other Linux
: Normal major
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-01-26 12:13 UTC by Jan Alexander Steffens (heftig)
Modified: 2017-01-26 21:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Replace g_byte_array_unref with g_free (988 bytes, patch)
2017-01-26 12:13 UTC, Jan Alexander Steffens (heftig)
none Details | Review
[PATCH] c-e: Fix bad unref causing heap corruption (1.94 KB, patch)
2017-01-26 19:45 UTC, Jan Alexander Steffens (heftig)
none Details | Review

Description Jan Alexander Steffens (heftig) 2017-01-26 12:13:39 UTC
Created attachment 344314 [details] [review]
Replace g_byte_array_unref with g_free

nm-connection-editor almost always crashes here when opening or closing a bridge connection window.

The "mac-address" property implemented by a few NMSetting subclasses is a string, not a boxed GByteArray. Attempting to free the string using g_byte_array_unref at page-master.c:208 causes heap corruption that crashes the connection editor at a random point later in malloc_consolidate or similarly.

valgrind caught the invalid read done by g_array_unref.

Proposed fix attached.
Comment 1 Jan Alexander Steffens (heftig) 2017-01-26 12:54:08 UTC
The patch also changes the logic to check if the mac-address is NULL or empty instead of just NULL, which might or might not be more correct. That was more of a hunch.
Comment 2 Thomas Haller 2017-01-26 13:09:26 UTC
yeah, patch is right. For the patch, which is your preferred writing for author? (in git-style).

Please consider creating future patches with `git format-patch`, which is really helpful as it entails a commit message composed by you + date + author.

thanks!!
Comment 3 Jan Alexander Steffens (heftig) 2017-01-26 13:27:34 UTC
Sorry, I normally do that, but I don't have my usual environment at the moment. I'll resubmit this later today with the proper style...
Comment 4 Jan Alexander Steffens (heftig) 2017-01-26 19:45:01 UTC
Created attachment 344351 [details] [review]
[PATCH] c-e: Fix bad unref causing heap corruption

The "mac-address" property implemented by a few NMSetting subclasses is
a string, not a boxed GByteArray.

Replace the g_byte_array_unref with a g_free. On a hunch, also check if
the returned string is empty instead of just NULL, which might be more
correct.

Using g_byte_array_unref on a string causes silent heap corruption,
resulting in a crash somewhere else in malloc_consolidate or similar.
For me, nm-connection-editor almost always crashed when opening or
closing the settings for an active bridge connection.

valgrind caught an invalid read done by g_array_unref, but neither gdb
nor "_MALLOC_CHECK=1 G_SLICE=always-malloc" was any help.
Comment 5 Thomas Haller 2017-01-26 21:42:50 UTC
merged to master: https://git.gnome.org/browse/network-manager-applet/commit/?id=e0bdcd687064121db35c3cede932f1ab0f157fe9

Thank you for reporting + fixing!!