From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 3C1573847821 for ; Fri, 16 Jul 2021 20:36:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3C1573847821 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-403-H77wKjOqNu6BvDnNmrs0Xg-1; Fri, 16 Jul 2021 16:36:53 -0400 X-MC-Unique: H77wKjOqNu6BvDnNmrs0Xg-1 Received: by mail-qv1-f70.google.com with SMTP id eo14-20020ad4594e0000b02902fc3fd31414so4037188qvb.16 for ; Fri, 16 Jul 2021 13:36:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=9tuo59gSCZX4dI4xPOoTpM2W+YI0OsEgGl8WQi+x1HY=; b=ae91caOrH/O/ZCcIV/zoah6g0KxPmJdVPrYNxJPpyOWuR0T9XZIZPoTw+zrfM8oXBJ KIjPy1HC3Z32eJwnTqwAVaKTxF7HxzAPdfFrXhSGbxwW3elS03INfR/2QRtZj/UhEldW qP4EZaZ0Wx61hUQhtX6usVVh6xCH+2hqt3uSxTO6qKJzPEFt/9JVv78hk8g6VE0hwFWk Ku9xBIMo3o0QaFiJOJkietyCZsj4rV4l7gTU3nB92C8fxmoIKCvylei0UPSXUFYOVoqW xGyAMY+qj1/fb8VUSCjeP7usGzExh3Sk2kInuudsYrJnfHmKB8L84+c8tgiPAtkibTxp 8xVA== X-Gm-Message-State: AOAM5305f4kvnuWuWtomZHoZGn0FKZOD8xYJcGL95mvkgt0afpiQRanp 5yKXEIR86qe1L/q/moObr/NdpRC6w3r63+wAnP8dGb2JrrzrYdfCwORId/C1ILtl1+0mYaZ+7+2 Lnt/Kcp37cpYPvDbAITkmoacSPcElo+5Jp7zFqgtYNU50GgFC99cncI+inYQCASbnzbQ//A== X-Received: by 2002:aed:306f:: with SMTP id 102mr10894440qte.197.1626467812320; Fri, 16 Jul 2021 13:36:52 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw4Na+xZcM5rO4YQoWAPeVy72gj0LNBZof3z+pN0UqLPhFdPLHOLIyzQLo7ZuCTnTxfnl94ww== X-Received: by 2002:aed:306f:: with SMTP id 102mr10894420qte.197.1626467812094; Fri, 16 Jul 2021 13:36:52 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id c190sm3881050qkg.46.2021.07.16.13.36.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 16 Jul 2021 13:36:51 -0700 (PDT) Subject: Re: [PATCH] tst-safe-linking: make false positives even more improbable To: Siddhesh Poyarekar , libc-alpha@sourceware.org References: <20210712153402.3966528-1-siddhesh@sourceware.org> From: Carlos O'Donell Organization: Red Hat Message-ID: <94c8ee00-c900-a086-d92c-99df6dbf94f4@redhat.com> Date: Fri, 16 Jul 2021 16:36:50 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210712153402.3966528-1-siddhesh@sourceware.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Jul 2021 20:36:55 -0000 On 7/12/21 11:34 AM, Siddhesh Poyarekar via Libc-alpha wrote: > There is a 1 in 16 chance of a corruption escaping safe-linking and to > guard against spurious failures, tst-safe-linking runs each subtest 10 > times to ensure that the chance is reduced to 1 in 2^40. However, in > the 1 in 16 chance that a corruption does escape safe linking, it > could well be caught by other sanity checks we do in malloc, which > then results in spurious test failures like below: > > test test_fastbin_consolidate failed with a different error > expected: malloc_consolidate(): unaligned fastbin chunk detected > > actual: malloc_consolidate(): invalid chunk size > > This failure is seen more frequently on i686; I was able to reproduce > it in about 5 min of running it in a loop. > > Guard against such failures by recording them and retrying the test. > Also, do not fail the test if we happened to get defeated by the 1 in > 2^40 odds if in at least one of the instances it was detected by other > checks. > > Finally, bolster the odds to 2^64 by running 16 times instead of 10. > The test still has a chance (about 1 in 2^40) of failure so it is > still flaky in theory. However in practice if we see a failure here > then it's more likely that there's a bug than it being an issue with > the test. Add more printfs and also dump them to stdout so that in > the event the test actually fails, we will have some data to try and > understand why it may have failed. OK for 2.34. Reviewed-by: Carlos O'Donell > --- > malloc/tst-safe-linking.c | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/malloc/tst-safe-linking.c b/malloc/tst-safe-linking.c > index 97cc108be6..959ba59704 100644 > --- a/malloc/tst-safe-linking.c > +++ b/malloc/tst-safe-linking.c > @@ -33,32 +33,39 @@ check (const char *test, void (*callback) (void *), > const char *expected) > { > int i, rand_mask; > - bool success = false; > + int success = 0; /* 0 == fail, 1 == other check 2 == safe linking */ OK. > /* There is a chance of 1/16 that a corrupted pointer will be aligned. > Try multiple times so that statistical failure will be improbable. */ > - for (i = 0; i < 10 && !success; ++i) > + for (i = 0; i < 16; ++i) OK. Iterate a little more. > { > rand_mask = rand () & 0xFF; > struct support_capture_subprocess result > = support_capture_subprocess (callback, &rand_mask); > + printf ("%s\n", result.out.buffer); OK. A little more verbose. > /* Did not crash, could happen. Try again. */ > if (strlen (result.err.buffer) == 0) > continue; > - /* Crashed, must be the expected result. */ > + /* Crashed, it may either be safe linking or some other check. If it's > + not safe linking then try again. */ > if (strcmp (result.err.buffer, expected) != 0) > { > - support_record_failure (); OK. Don't record a failure. > - printf ("error: test %s unexpected standard error data\n" > + printf ("test %s failed with a different error\n" > " expected: %s\n" > " actual: %s\n", > test, expected, result.err.buffer); > + success = 1; OK. Some other malloc check caught the failure. > + continue; > } > TEST_VERIFY (WIFSIGNALED (result.status)); > if (WIFSIGNALED (result.status)) > TEST_VERIFY (WTERMSIG (result.status) == SIGABRT); > support_capture_subprocess_free (&result); > - success = true; > + success = 2; OK. safe-linking caught the failure. > + break; > } > + /* The test fails only if the corruption was not caught by any of the malloc > + mechanisms in all those iterations. This has a lower than 1 in 2^64 > + chance of a false positive. */ > TEST_VERIFY (success); > } > > @@ -74,10 +81,13 @@ test_tcache (void *closure) > int mask = ((int *)closure)[0]; > size_t size = TCACHE_ALLOC_SIZE; > > + printf ("++ tcache ++\n"); > + > /* Populate the tcache list. */ > void * volatile a = malloc (size); > void * volatile b = malloc (size); > void * volatile c = malloc (size); > + printf ("a=%p, b=%p, c=%p\n", a, b, c); > free (a); > free (b); > free (c); > @@ -88,6 +98,7 @@ test_tcache (void *closure) > printf ("After: c=%p, c[0]=%p\n", c, ((void **)c)[0]); > > c = malloc (size); > + printf ("Allocated: c=%p\n", c); > /* This line will trigger the Safe-Linking check. */ > b = malloc (size); > printf ("b=%p\n", b); > @@ -101,10 +112,13 @@ test_fastbin (void *closure) > int mask = ((int *)closure)[0]; > size_t size = TCACHE_ALLOC_SIZE; > > + printf ("++ fastbin ++\n"); > + > /* Take the tcache out of the game. */ > for (i = 0; i < TCACHE_FILL_COUNT; ++i) > { > void * volatile p = calloc (1, size); > + printf ("p=%p\n", p); > free (p); > } > > @@ -112,6 +126,7 @@ test_fastbin (void *closure) > void * volatile a = calloc (1, size); > void * volatile b = calloc (1, size); > void * volatile c = calloc (1, size); > + printf ("a=%p, b=%p, c=%p\n", a, b, c); > free (a); > free (b); > free (c); > @@ -122,6 +137,7 @@ test_fastbin (void *closure) > printf ("After: c=%p, c[0]=%p\n", c, ((void **)c)[0]); > > c = calloc (1, size); > + printf ("Allocated: c=%p\n", c); > /* This line will trigger the Safe-Linking check. */ > b = calloc (1, size); > printf ("b=%p\n", b); > @@ -135,6 +151,8 @@ test_fastbin_consolidate (void *closure) > int mask = ((int*)closure)[0]; > size_t size = TCACHE_ALLOC_SIZE; > > + printf ("++ fastbin consolidate ++\n"); > + > /* Take the tcache out of the game. */ > for (i = 0; i < TCACHE_FILL_COUNT; ++i) > { > @@ -146,6 +164,7 @@ test_fastbin_consolidate (void *closure) > void * volatile a = calloc (1, size); > void * volatile b = calloc (1, size); > void * volatile c = calloc (1, size); > + printf ("a=%p, b=%p, c=%p\n", a, b, c); OK. > free (a); > free (b); > free (c); > -- Cheers, Carlos.