diff mbox series

Fix regression in memrchr

Message ID PAVPR08MB884565C30F34D3545E591070F94FA@PAVPR08MB8845.eurprd08.prod.outlook.com
State New
Headers
Series Fix regression in memrchr |

Commit Message

Federico Terraneo March 19, 2026, 7:55 a.m. UTC 
  Hi,
while upgrading our toolchain for the Miosix kernel to use the lastest 
Newlib release, we noticed a regression in memrchr. This issue causes an 
unaligned memory access resulting in a fault in ARM Cortex M0 
microcontrollers.

The bug has been introduced in commit 
c9b74e32892966c8f66280c752181292bc95f690, where the macro UNALIGNED was 
replaced with a generic one UNALIGNED_X.
However, in memrchr.c the UNALIGNED macro was defined as

#define UNALIGNED(X) ((long)(X + 1) & (sizeof (long) - 1))

thus checking if X+1 is aligned, not if X is aligned.

The replacement macro checks if X is aligned, thus breaking the algorithm.

The issue was found by my colleague Daniele Cattaneo.

Best regards,
Federico Terraneo

Comments

Corinna Vinschen March 19, 2026, 10:59 a.m. UTC | #1 
CC the author of c9b74e328, Alexey Lapshin.

Alexey, any input?


Thanks,
Corinna




On Mar 19 08:55, Federico Terraneo wrote:
> Hi,
> while upgrading our toolchain for the Miosix kernel to use the lastest
> Newlib release, we noticed a regression in memrchr. This issue causes an
> unaligned memory access resulting in a fault in ARM Cortex M0
> microcontrollers.
> 
> The bug has been introduced in commit
> c9b74e32892966c8f66280c752181292bc95f690, where the macro UNALIGNED was
> replaced with a generic one UNALIGNED_X.
> However, in memrchr.c the UNALIGNED macro was defined as
> 
> #define UNALIGNED(X) ((long)(X + 1) & (sizeof (long) - 1))
> 
> thus checking if X+1 is aligned, not if X is aligned.
> 
> The replacement macro checks if X is aligned, thus breaking the algorithm.
> 
> The issue was found by my colleague Daniele Cattaneo.
> 
> Best regards,
> Federico Terraneo

> From ea9b7d9f09d48b2bf88682d47992f5ee0bd4e57d Mon Sep 17 00:00:00 2001
> From: Daniele Cattaneo <daniele.cattaneo@polimi.it>
> Date: Wed, 18 Mar 2026 17:33:02 +0100
> Subject: [PATCH] libc: string: Fix off-by-one alignment bug in memrchr.
> 
> The loop at line 50 should test the bytes in the buffer, from the end towards
> the beginning, until it reaches a character whose address is aligned, leaving
> `src' pointing to the next character to test. However, the loop condition did
> not test the address of the last character read (`src + 1'), but the address of
> the next character (`src'). As a consequence, a misaligned address was computed
> at line 69, because the (aligned) address in `src' is incremented by 1.
> 
> This bug was originally introduced in commit c9b74e328 during a refactoring of
> the macros used by string functions. Before the refactoring, the +1 increment
> in the loop condition was located in the macros specific to memrchr.
> ---
>  newlib/libc/string/memrchr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/newlib/libc/string/memrchr.c b/newlib/libc/string/memrchr.c
> index 0a0c80fd9..b56e2b476 100644
> --- a/newlib/libc/string/memrchr.c
> +++ b/newlib/libc/string/memrchr.c
> @@ -47,7 +47,7 @@ memrchr (const void *src_void,
>    unsigned long  mask;
>    unsigned int i;
>  
> -  while (UNALIGNED_X(src))
> +  while (UNALIGNED_X(src + 1))
>      {
>        if (!length--)
>          return NULL;
> -- 
> 2.53.0
>
Jeff Johnston March 19, 2026, 3:21 p.m. UTC | #2 
Looking at the patch from Alexey, he indeed missed that memrchr used a
different UNALIGNED macro definition and replacing it with the common one
needs the change specified.

Patch approved.  Thanks.

-- Jeff J.


On Thu, Mar 19, 2026 at 6:59 AM Corinna Vinschen <corinna@vinschen.de>
wrote:

> CC the author of c9b74e328, Alexey Lapshin.
>
> Alexey, any input?
>
>
> Thanks,
> Corinna
>
>
>
>
> On Mar 19 08:55, Federico Terraneo wrote:
> > Hi,
> > while upgrading our toolchain for the Miosix kernel to use the lastest
> > Newlib release, we noticed a regression in memrchr. This issue causes an
> > unaligned memory access resulting in a fault in ARM Cortex M0
> > microcontrollers.
> >
> > The bug has been introduced in commit
> > c9b74e32892966c8f66280c752181292bc95f690, where the macro UNALIGNED was
> > replaced with a generic one UNALIGNED_X.
> > However, in memrchr.c the UNALIGNED macro was defined as
> >
> > #define UNALIGNED(X) ((long)(X + 1) & (sizeof (long) - 1))
> >
> > thus checking if X+1 is aligned, not if X is aligned.
> >
> > The replacement macro checks if X is aligned, thus breaking the
> algorithm.
> >
> > The issue was found by my colleague Daniele Cattaneo.
> >
> > Best regards,
> > Federico Terraneo
>
> > From ea9b7d9f09d48b2bf88682d47992f5ee0bd4e57d Mon Sep 17 00:00:00 2001
> > From: Daniele Cattaneo <daniele.cattaneo@polimi.it>
> > Date: Wed, 18 Mar 2026 17:33:02 +0100
> > Subject: [PATCH] libc: string: Fix off-by-one alignment bug in memrchr.
> >
> > The loop at line 50 should test the bytes in the buffer, from the end
> towards
> > the beginning, until it reaches a character whose address is aligned,
> leaving
> > `src' pointing to the next character to test. However, the loop
> condition did
> > not test the address of the last character read (`src + 1'), but the
> address of
> > the next character (`src'). As a consequence, a misaligned address was
> computed
> > at line 69, because the (aligned) address in `src' is incremented by 1.
> >
> > This bug was originally introduced in commit c9b74e328 during a
> refactoring of
> > the macros used by string functions. Before the refactoring, the +1
> increment
> > in the loop condition was located in the macros specific to memrchr.
> > ---
> >  newlib/libc/string/memrchr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/newlib/libc/string/memrchr.c b/newlib/libc/string/memrchr.c
> > index 0a0c80fd9..b56e2b476 100644
> > --- a/newlib/libc/string/memrchr.c
> > +++ b/newlib/libc/string/memrchr.c
> > @@ -47,7 +47,7 @@ memrchr (const void *src_void,
> >    unsigned long  mask;
> >    unsigned int i;
> >
> > -  while (UNALIGNED_X(src))
> > +  while (UNALIGNED_X(src + 1))
> >      {
> >        if (!length--)
> >          return NULL;
> > --
> > 2.53.0
> >
>
>
diff mbox series

Patch

From ea9b7d9f09d48b2bf88682d47992f5ee0bd4e57d Mon Sep 17 00:00:00 2001
From: Daniele Cattaneo <daniele.cattaneo@polimi.it>
Date: Wed, 18 Mar 2026 17:33:02 +0100
Subject: [PATCH] libc: string: Fix off-by-one alignment bug in memrchr.

The loop at line 50 should test the bytes in the buffer, from the end towards
the beginning, until it reaches a character whose address is aligned, leaving
`src' pointing to the next character to test. However, the loop condition did
not test the address of the last character read (`src + 1'), but the address of
the next character (`src'). As a consequence, a misaligned address was computed
at line 69, because the (aligned) address in `src' is incremented by 1.

This bug was originally introduced in commit c9b74e328 during a refactoring of
the macros used by string functions. Before the refactoring, the +1 increment
in the loop condition was located in the macros specific to memrchr.
---
 newlib/libc/string/memrchr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/newlib/libc/string/memrchr.c b/newlib/libc/string/memrchr.c
index 0a0c80fd9..b56e2b476 100644
--- a/newlib/libc/string/memrchr.c
+++ b/newlib/libc/string/memrchr.c
@@ -47,7 +47,7 @@  memrchr (const void *src_void,
   unsigned long  mask;
   unsigned int i;
 
-  while (UNALIGNED_X(src))
+  while (UNALIGNED_X(src + 1))
     {
       if (!length--)
         return NULL;
-- 
2.53.0