From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-9.2 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,URIBL_BLACK,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 25567 invoked from network); 30 Aug 2021 14:54:34 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 30 Aug 2021 14:54:34 -0000 Received: (qmail 24246 invoked by uid 550); 30 Aug 2021 14:54:32 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 17670 invoked from network); 30 Aug 2021 14:37:46 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=nwLkQl0PQ74q+NDAW2HSWJR1H6WzekFxt1qJos0mZZc=; b=f9mw2QP2ssp0Bda9T3kaTGWaTL2k45fhIN2V2euBEHAKT0rBszLNvn8fTlgq+Ap/sJ FZVGr1x2eSpG3nU/M+ajm4J21KZDHYsSnptluS8gZvmCJvk/MfPGqeeAP8IE6IwNsv/a u0D9SO1CYhY9eEr/wlYWLHh4BubWxIrsTcG+wa5Gj4fRBB3g1VwJPcNTp9L3pUSormFN m0cxk7fsgHlj4Tyy6PS90BiF5MYYSKeFcBL03149Z1+Eu0E2vvvvVgR0Usar0t5s9HJT JyNvNZMv6dYPyjGc4Rkd/0HSdApHiaiz0pPHnOdkzegmlvNswh7ipA8EVeQKHfXgF3NB lByQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=nwLkQl0PQ74q+NDAW2HSWJR1H6WzekFxt1qJos0mZZc=; b=H1+kmIadVWtW7bgpT03mef5r+Hfi+mWKyTO20r/oLpa97M4n0bvLIS9s8mBBLQ9YsJ 1xI4vcj1cOh2J2wahFlY/r1pEmTItl52OmE1W7Lfre0ht9KGI+munpRDJwB8indlyYIC 9lgPseFKjq1BojVRPno9c2p6bWUFz3Ec4rH1xIC0UHiv29Tc2MjJDgElD0R7iE/9+8KN z73g8IW3UB4DPMKsHW1u3i5bt1VV6zzcOg0dK6pUQdCRMSl2rQ7+tmSdQnDcARvda1i8 Ekxfulc+9H6Yvq2ZhqgKpb42pPL8QazeTYTziceXCrYXXWd8Ilox4+Tl+jhWyPKLzwmv Gp7Q== X-Gm-Message-State: AOAM531DsFlOi82vHrA1LvtTOx1r1dPMYtcsIVx9rv6QhTha/uktpGQs MZmULcbS8i4hnlmCBHL0bPbQKVk2ADr28/dA7+Zu X-Google-Smtp-Source: ABdhPJyV23ReMmx4fZvEqC62VYLin4EsGYEGzJ91dD5waPMmmEL+lrOUDgvuoSIiJgIdS9+5VT2Blb6YNfEwQ/kRWdE= X-Received: by 2002:a05:6402:b85:: with SMTP id cf5mr12126119edb.153.1630334254430; Mon, 30 Aug 2021 07:37:34 -0700 (PDT) MIME-Version: 1.0 References: <20210830121736.GB13220@brightrain.aerifal.cx> In-Reply-To: <20210830121736.GB13220@brightrain.aerifal.cx> From: Tamir Duberstein Date: Mon, 30 Aug 2021 10:37:22 -0400 Message-ID: To: Rich Felker Cc: musl@lists.openwall.com, Petr Hosek Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [musl] undefined behavior in getdelim.c On Mon, Aug 30, 2021 at 8:17 AM Rich Felker wrote: > > On Sun, Aug 29, 2021 at 06:13:44PM -0400, Tamir Duberstein wrote: > > Fuchsia's libc is derived from musl. We make extensive use of clang > > sanitizers in Fuchsia, and UBSAN has found "applying zero offset to > > null pointer" in getdelim.c. > > Thank you for the detailed report! > > > Any call to `fopen` followed by a call to `getdelim` will trigger this > > behavior. The UB happens at > > https://git.musl-libc.org/cgit/musl/tree/src/stdio/getdelim.c#n59. > > Immediately after `fopen` `f->rpos` is `NULL`; `rpos` won't be > > initialized until a few lines down in `getcunlocked`. > > So, a code path where p+=3Dk where p is a null pointer and k is 0, > right? Yup. > > Here's the stack trace from UBSAN in Fuchsia: > > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48:13: runtime > > error: applying zero offset to null pointer > > #0 0x0000432ff5bf0613 in getdelim(char** restrict, size_t* > > restrict, int, FILE* restrict) > > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48 > > +0x165613 > > #1.2 0x00002380af30fe37 in ubsan_GetStackTrace() > > compiler-rt/lib/ubsan/ubsan_diag.cpp:55 +0x3be37 > > #1.1 0x00002380af30fe37 in MaybePrintStackTrace() > > compiler-rt/lib/ubsan/ubsan_diag.cpp:53 +0x3be37 > > #1 0x00002380af30fe37 in ~ScopedReport() > > compiler-rt/lib/ubsan/ubsan_diag.cpp:389 +0x3be37 > > #2 0x00002380af3141fb in handlePointerOverflowImpl() > > compiler-rt/lib/ubsan/ubsan_handlers.cpp:809 > > +0x401fb > > #3 0x00002380af313d6d in > > compiler-rt/lib/ubsan/ubsan_handlers.cpp:815 > > +0x3fd6d > > #4 0x0000432ff5bf0613 in getdelim(char** restrict, size_t* > > restrict, int, FILE* restrict) > > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48 > > +0x165613 > > > > Note that Fuchsia is a years behind, but I've confirmed this UB > > happens even with the latest musl sources. > > > > Fixing this should be quite straightforward. I'm happy to send a patch > > if you agree. > > > > Please CC me on response as I am not a subscriber to this mailing list > > per the guidance on https://musl.libc.org/support.html. > > What fix do you have in mind? I believe like 58 is also UB, by virtue > of passing a null pointer to memcpy, albeit with zero length. My first > leaning would be to get rid of the else body at line 32-33 and instead > goto line with getc_unlocked, but the reason that's not already being > done is that the buffer growth code at least originally needed to be > hit before the getc_unlocked. I don't recall immediately if that's > still the case. A very safe approach would be just putting lines 58-60 > inside "if (k) { ... }" Good point, 58 is UB according to ISO/IEC 9899:1999 (http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf) =C2=A77.21.1: Where an argument declared as size_t n specifies the length of the array fo= r a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. where =C2=A77.1.4.1 states: If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer, or a pointer to non-modifiable storage when the correspo= nding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined. I think the safe choice of just wrapping it in an `if (k) { ... }` is the way to go. It would be nice to add a test case to libc-test for this trivial case. Is there precedent for file operations in libc-test? As far as I can tell the only stdio tests in libc-test are signature tests (http://nsz.repo.hu/git/?p=3Dlibc-test;a=3Dblob;f=3Dsrc/api/stdio.c;hb=3DHE= AD). By the way, where is the infrastructure that runs libc-test against musl? Is UBSAN enabled there? Tamir