Created attachment 1624 [details] address sanitizer error message for use after free bug I'm testing claws-mail with address sanitizer. Today it found a use-after-free error. Unfortunately I was unable to reproduce it (it happened while deleting a couple of mails at once, but I am not certain about the exact conditions). I thought I'd better report it anyway, the asan error message is pretty detailed. I tried to look into the code, but was unable to figure out what might go wrong.
I am not familiar with how exactly asan works, but from the attachment I get the impression that it overwrites (marks) freed heap memory areas with arbitrary bytes. Could this bug then be triggered by procmsg_msginfo_free() not zeroing out pointers to memory it frees? I can see that e.g. in summary_execute_move_func(), if the "msginfo" pointer points to a freed memory (perhaps freed by messageview_clear(), called from summary_unthread_for_exec(), which itself is called one or two stack frames above and before summary_execute_move_func()), and asan overwriting the freed memory with 0xfd, how most of the conditions touching the "msginfo" pointer in this functions would evaluate to true: line 5185: if (msginfo && MSG_IS_MOVE(msginfo->flags) && msginfo->to_folder) { line 5197: if (prefs_common.thread_by_subject && msginfo->subject && *msginfo->subject && node == subject_table_lookup(summaryview->subject_table, msginfo->subject)) { Possible solution: perhaps have procmsg_msginfo_free() assign NULL to each pointer to memory it frees? E.g.: g_free(msginfo->fromspace); msginfo->fromspace = NULL;
What asan does is esentially that it records freed memory and will throw an error if you try to access it. Your code lines both seem problematic: line 5185: if (msginfo && MSG_IS_MOVE(msginfo->flags) && msginfo->to_folder) { This seems to assume that "if (msginfo" will tell you whether msginfo contains a valid object, i.e. it must contain a null pointer if it doesn't. In order to have this guarantee you must a) pre-initialize the pointer with null b) make sure on every free it get's assigned null There are many such checks in the code. line 5197: if (prefs_common.thread_by_subject && msginfo->subject && *msginfo->subject && node == subject_table_lookup(summaryview->subject_table, msginfo->subject)) { This is the same issue, except that it seems to expect msginfo->subject to be always null. This seems like a bigger issue that is scattered throughout the codebase... Essentially each line like this: procmsg_msginfo_free(msginfo); would need a msginfo = NULL; after it. Not sure how to start tackling this, it seems a lot of work. (But would probably increase overall stability, these uaf issues can lead to weird instability behavior.)
That's (In reply to comment #2) > Essentially each line like this: > procmsg_msginfo_free(msginfo); > would need a > msginfo = NULL; > after it. > > Not sure how to start tackling this, it seems a lot of work. (But would > probably increase overall stability, these uaf issues can lead to weird > instability behavior.) Yes, that was my proposed solution. I think a "msginfo = NULL;" line at the end of procmsg_msginfo_free() should be enough to fix this particular use-after-free instance, as that function is the only way we use to free the heap-allocated MsgInfo structs. Setting all the pointer members of MsgInfo to NULL in procmsg_msginfo_free() is a nice safety bonus, and shouldn't incur much performance penalty.
(In reply to comment #3) > > Yes, that was my proposed solution. I think a "msginfo = NULL;" line at the > end of procmsg_msginfo_free() should be enough to fix this particular > use-after-free instance, as that function is the only way we use to free the > heap-allocated MsgInfo structs. > > Setting all the pointer members of MsgInfo to NULL in procmsg_msginfo_free() > is a nice safety bonus, and shouldn't incur much performance penalty. You would still need to assign NULL to your own pointer since adding msginfo = NULL in procmsg_msginfo_free will only operate on what your a pointing to and not your own pointer.
>You would still need to assign NULL to your own pointer since adding msginfo = >NULL in procmsg_msginfo_free will only operate on what your a pointing to and >not your own pointer. Exactly. That's what makes it complicated. There are two ways out: a) change procmsg_msginfo_free() to not operate on a pointer, but a pointer to a pointer (**msginfo). Then you can pass &msginfo and zero that. b) change each call of procmsg_msginfo_free() to have a subsequent msginfo = NULL. Option a) has the additional downside that this requires you to make this change also within plugins. Both options don't look very nice. There may be also option c) add a new function procmsg_msginfo_free_pointer() which: * calls procmsg_msginfo_free() * sets the given pointer to NULL This wouldn't outright break all existing code and it could be migrated slowly. At some point you might then want to deprecate calling procmsg_msginfo_free directly.
Yes, thanks to both of you, I wasn't thinking clearly there. :) As we do not have any plugins that we know of outside of our main source tree (in src/plugins/), I would prefer option A, since the mentioned downside is non-existent - the plugins' code can get updated at the same time as the core, it's just slightly more search&replace work.
Changes related to this bug have been committed. Please check latest Git and update the bug accordingly. You can also get the patch from: http://git.claws-mail.org/ ++ ChangeLog 2016-01-23 16:51:02.978908057 +0100 http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=fba615e2e1f19b953304521c4fbbf05f2bad70ef Merge: 5f3008d bfb1815 Author: Colin Leroy <colin@colino.net> Date: Sat Jan 23 16:51:02 2016 +0100 Merge branch 'master' of file:///home/git/claws http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=bfb18150efae6fc3eb1c7f436e1ffc86c8e375fa Author: Andrej Kacian <ticho@claws-mail.org> Date: Sat Jan 23 15:40:38 2016 +0100 Make procmsg_msginfo_free() zero out pointers to freed memory. The function's argument type changes from MsgInfo* to MsgInfo**, so that we can zero out the pointer. This closes bug #3598, reported by Hanno Boeck.
Thanks again, Hanno.
Unfortunately it seems this fix is not enough. I am still able to reproduce this issue with the latest git code. I assume the reason is that the pointer is copied around at some point and although it's nulled on freeing the copy will still be accessed. I'm attaching an asan crash dump. Not sure how to debug this further. Unfortunately I'm still not able to reliably reproduce this, but it happens usually after moving around and deleting a few messages in my inbox.
Created attachment 1639 [details] asan error of use after free with latest git code
(In reply to comment #9) > Unfortunately it seems this fix is not enough. I am still able to reproduce > this issue with the latest git code. > > I assume the reason is that the pointer is copied around at some point and > although it's nulled on freeing the copy will still be accessed. I'm > attaching an asan crash dump. > > Not sure how to debug this further. Unfortunately I'm still not able to > reliably reproduce this, but it happens usually after moving around and > deleting a few messages in my inbox. It seems the problem is that in folder.c the msginfo->subject is inserted in the subject GHashTable without being g_strdup'ed (using utils.c:subject_table_insert). If freed by procmsg_msginfo_free, the utils.c:subject_table_lookup function will try to access a freed string, as the asan report shows. HTH,