[PATCH] top: fix double free causing a SIGABRT storm after SIGPIPE

[Denys Vlasenko]

On Thu, Aug 18, 2016 at 11:25 PM, Luca Ceresoli <luca at lucaceresoli.net> wrote:
> Hi,
>
> On 18/08/2016 23:04, Luca Ceresoli wrote:
>> On some platforms the command 'top -n1 | head' goes very often into an
>> infinite loop of SIGABRT and double free()s.
>>
>> This happens only if FEATURE_TOP_CPU_USAGE_PERCENTAGE,
>> CONFIG_FEATURE_CLEAN_UP and FEATURE_USE_TERMIOS are all set.
>>
>> What happens is that, at the very end of top_main(), reset_term() is
>> called to free allocated memory. reset_term() calls free(prev_hist)
>> without setting prev_hist to NULL, which _looks_ OK since we're about
>> to exit.
>>
>> Immediately after we receive SIGPIPE because 'tail' discarded some of
>> our output. Thus the sig_catcher() handler is called, which calls
>> reset_term() to free allocated memory... which calls free(prev_hist)
>> again on the same address. glibc detects the double free and
>> complains:
>>
>>   *** Error in `top': double free or corruption (!prev): 0x000c9110 ***
>>
>> And here we get SIGABRT. sig_catcher() is invoked again, leading to
>> another free(prev_hist). This repeats for several minutes, until the
>> process get a SIGSEGV and is finally killed.
>>
>> Fix the bug by setting prev_hist to NULL after free()ing it.
>
> While working to this fix I also noticed that both sig_catcher() and
> reset_term() (which frees prev_hist()) are under #if
> ENABLE_FEATURE_USE_TERMIOS. This means some memory is not freed when not
> using termios, although that memory has no relationship with terminal at
> all.
>
> Well, this has no practical effect since we are about to exit, but
> anyway I think this is wrong. Memory should be freed before exiting if
> and only if ENABLE_FEATURE_CLEAN_UP=y. I'll have a deeper look and
> probably will send a patch in a few days.

Let's just move freeing code out of signal handling path?


@@ -728,12 +728,6 @@ static void reset_term(void)
 {
        if (!OPT_BATCH_MODE)
                tcsetattr_stdin_TCSANOW(&initial_settings);
-       if (ENABLE_FEATURE_CLEAN_UP) {
-               clearmems();
-# if ENABLE_FEATURE_TOP_CPU_USAGE_PERCENTAGE
-               free(prev_hist);
-# endif
-       }
 }

 static void sig_catcher(int sig)
@@ -1258,5 +1252,11 @@ int top_main(int argc UNUSED_PARAM, char **argv)
 #if ENABLE_FEATURE_USE_TERMIOS
        reset_term();
 #endif
+       if (ENABLE_FEATURE_CLEAN_UP) {
+               clearmems();
+#if ENABLE_FEATURE_TOP_CPU_USAGE_PERCENTAGE
+               free(prev_hist);
+#endif
+       }
        return EXIT_SUCCESS;
 }
>
> --
> Luca
> _______________________________________________
> busybox mailing list
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox