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=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 11691 invoked from network); 3 Jul 2023 20:00:13 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 3 Jul 2023 20:00:13 -0000 Received: (qmail 8088 invoked by uid 550); 3 Jul 2023 20:00:10 -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 8041 invoked from network); 3 Jul 2023 20:00:09 -0000 Date: Mon, 3 Jul 2023 15:59:57 -0400 From: Rich Felker To: "Alex Xu (Hello71)" Cc: musl@lists.openwall.com Message-ID: <20230703195957.GZ4163@brightrain.aerifal.cx> References: <1688401586.hkqjuyrd3s.none.ref@localhost> <1688401586.hkqjuyrd3s.none@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1688401586.hkqjuyrd3s.none@localhost> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] fix various warnings/theoretical UB On Mon, Jul 03, 2023 at 01:55:57PM -0400, Alex Xu (Hello71) wrote: > See attached patches. > From 978f2cded65ce73450277d3fde48f038b339d5f9 Mon Sep 17 00:00:00 2001 > From: "Alex Xu (Hello71)" > Date: Sun, 2 Jul 2023 20:28:23 -0400 > Subject: [PATCH 1/4] volatile static -> static volatile > > C11 6.11.5p1: > > > The placement of a storage-class specifier other than at the > > beginning of the declaration specifiers in a declaration is an > > obsolescent feature. > > gcc also warns about this. > --- > src/time/timer_create.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/time/timer_create.c b/src/time/timer_create.c > index cd32c945..9216b3ab 100644 > --- a/src/time/timer_create.c > +++ b/src/time/timer_create.c > @@ -61,7 +61,7 @@ static void *start(void *arg) > > int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict res) > { > - volatile static int init = 0; > + static volatile int init = 0; > pthread_t td; > pthread_attr_t attr; > int r; > -- > 2.41.0 No objection to this change. It's contrary to usual style. I would say let's convert to pthread_once, but this code is slated for removal anyway once signals are no longer used for SIGEV_THREAD timers. > From b98f243e7921ddff6978ee9b0ce9f08efaa17951 Mon Sep 17 00:00:00 2001 > From: "Alex Xu (Hello71)" > Date: Sun, 2 Jul 2023 20:29:41 -0400 > Subject: [PATCH 2/4] __year_to_secs: fix dangling pointer > > C11 6.5.2.5p5: > > > If the compound literal occurs outside the body of a function, the > > object has static storage duration; otherwise, it has automatic > > storage duration associated with the enclosing block. > > gcc also warns about this. > --- > src/time/__year_to_secs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/time/__year_to_secs.c b/src/time/__year_to_secs.c > index 2824ec6d..d215880a 100644 > --- a/src/time/__year_to_secs.c > +++ b/src/time/__year_to_secs.c > @@ -10,9 +10,9 @@ long long __year_to_secs(long long year, int *is_leap) > return 31536000*(y-70) + 86400*leaps; > } > > - int cycles, centuries, leaps, rem; > + int cycles, centuries, leaps, rem, tmp; > > - if (!is_leap) is_leap = &(int){0}; > + if (!is_leap) is_leap = &tmp; > cycles = (year-100) / 400; > rem = (year-100) % 400; > if (rem < 0) { > -- > 2.41.0 Seems like a bogus warning. The enclosing block is the whole function, the same as the lifetime of the pointer. This might merit investigation on whether GCC is doing something wrong though.. > From a30c4ab397af040d10d978d97dd4a6835d4b99a8 Mon Sep 17 00:00:00 2001 > From: "Alex Xu (Hello71)" > Date: Sun, 2 Jul 2023 20:54:45 -0400 > Subject: [PATCH 3/4] fix mismatched VLA parameter types > > gcc warns about this, and it's probably technically UB > --- > src/internal/procfdname.c | 2 +- > src/prng/seed48.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/internal/procfdname.c b/src/internal/procfdname.c > index fd7306ab..bfa3e7e5 100644 > --- a/src/internal/procfdname.c > +++ b/src/internal/procfdname.c > @@ -1,6 +1,6 @@ > #include "syscall.h" > > -void __procfdname(char *buf, unsigned fd) > +void __procfdname(char buf[static 15+3*sizeof(int)], unsigned fd) > { > unsigned i, j; > for (i=0; (buf[i] = "/proc/self/fd/"[i]); i++); This was raised/proposed before and is probably an okay change, but I'd like to understand what the reason "it's probably technically UB" is. > diff --git a/src/prng/seed48.c b/src/prng/seed48.c > index bce7b339..7b789086 100644 > --- a/src/prng/seed48.c > +++ b/src/prng/seed48.c > @@ -2,7 +2,7 @@ > #include > #include "rand48.h" > > -unsigned short *seed48(unsigned short *s) > +unsigned short *seed48(unsigned short s[3]) > { > static unsigned short p[3]; > memcpy(p, __seed48, sizeof p); > -- This one is almost surely not UB because there's no static and the 3 is ignored. The question is just whether the static produces a difference in the declaration type that makes them clash. Rich