| Message ID | 20241111224832.911593-1-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] elf: handle addition overflow in _dl_find_object_update_1 [BZ #32245] | expand |
On 2024-11-11 5:48 p.m., Aurelien Jarno wrote: > The remaining_to_add variable can be 0 if (current_used + count) wraps, > This is caught by GCC 14+ on hppa, which determines from there that > target_seg could be be NULL when remaining_to_add is zero, which in > turns causes a -Wstringop-overflow warning: > > In file included from ../include/atomic.h:49, > from dl-find_object.c:20: > In function '_dlfo_update_init_seg', > inlined from '_dl_find_object_update_1' at dl-find_object.c:689:30, > inlined from '_dl_find_object_update' at dl-find_object.c:805:13: > ../sysdeps/unix/sysv/linux/hppa/atomic-machine.h:44:4: error: '__atomic_store_4' writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=] > 44 | __atomic_store_n ((mem), (val), __ATOMIC_RELAXED); \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > dl-find_object.c:644:3: note: in expansion of macro 'atomic_store_relaxed' > 644 | atomic_store_relaxed (&seg->size, new_seg_size); > | ^~~~~~~~~~~~~~~~~~~~ > In function '_dl_find_object_update': > cc1: note: destination object is likely at address zero > > In practice, this is not possible as it represent counts of link maps. > Link maps have sizes larger than 1 byte, so the sum of any two link map > counts will always fit within a size_t without wrapping around. > > This patch therefore adds a check on remaining_to_add == 0 and tell GCC > that this can not happen using __builtin_unreachable. > > Thanks to Andreas Schwab for the investigation. > > Closes: BZ #32245 > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> Except for comment, tested-by: John David Anglin <dave.anglin@bell.net> > --- > elf/dl-find_object.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/elf/dl-find_object.c b/elf/dl-find_object.c > index 449302eda3..ae18b438d3 100644 > --- a/elf/dl-find_object.c > +++ b/elf/dl-find_object.c > @@ -662,6 +662,14 @@ _dl_find_object_update_1 (struct link_map **loaded, size_t count) > = _dlfo_loaded_mappings[!active_idx]; > size_t remaining_to_add = current_used + count; > > + /* remaining_to_add can be 0 if (current_used + count) wraps, but in practice > + this is not possible as it represent counts of link maps. Link maps have > + sizes larger than 1 byte, so the sum of any two link map counts will > + always fit within a size_t without wrapping around. This check ensures > + that target_seg is not erroneously considered potentially NULL by GCC. */ > + if (remaining_to_add == 0) > + __builtin_unreachable (); > + > /* Ensure that the new segment chain has enough space. */ > { > size_t new_allocated
On Nov 11 2024, Aurelien Jarno wrote: > The remaining_to_add variable can be 0 if (current_used + count) wraps, > This is caught by GCC 14+ on hppa, which determines from there that > target_seg could be be NULL when remaining_to_add is zero, which in > turns causes a -Wstringop-overflow warning: > > In file included from ../include/atomic.h:49, > from dl-find_object.c:20: > In function '_dlfo_update_init_seg', > inlined from '_dl_find_object_update_1' at dl-find_object.c:689:30, > inlined from '_dl_find_object_update' at dl-find_object.c:805:13: > ../sysdeps/unix/sysv/linux/hppa/atomic-machine.h:44:4: error: '__atomic_store_4' writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=] > 44 | __atomic_store_n ((mem), (val), __ATOMIC_RELAXED); \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > dl-find_object.c:644:3: note: in expansion of macro 'atomic_store_relaxed' > 644 | atomic_store_relaxed (&seg->size, new_seg_size); > | ^~~~~~~~~~~~~~~~~~~~ > In function '_dl_find_object_update': > cc1: note: destination object is likely at address zero > > In practice, this is not possible as it represent counts of link maps. > Link maps have sizes larger than 1 byte, so the sum of any two link map > counts will always fit within a size_t without wrapping around. > > This patch therefore adds a check on remaining_to_add == 0 and tell GCC > that this can not happen using __builtin_unreachable. Ok.
* Aurelien Jarno: > The remaining_to_add variable can be 0 if (current_used + count) wraps, > This is caught by GCC 14+ on hppa, which determines from there that > target_seg could be be NULL when remaining_to_add is zero, which in > turns causes a -Wstringop-overflow warning: > > In file included from ../include/atomic.h:49, > from dl-find_object.c:20: > In function '_dlfo_update_init_seg', > inlined from '_dl_find_object_update_1' at dl-find_object.c:689:30, > inlined from '_dl_find_object_update' at dl-find_object.c:805:13: > ../sysdeps/unix/sysv/linux/hppa/atomic-machine.h:44:4: error: '__atomic_store_4' writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=] > 44 | __atomic_store_n ((mem), (val), __ATOMIC_RELAXED); \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > dl-find_object.c:644:3: note: in expansion of macro 'atomic_store_relaxed' > 644 | atomic_store_relaxed (&seg->size, new_seg_size); > | ^~~~~~~~~~~~~~~~~~~~ > In function '_dl_find_object_update': > cc1: note: destination object is likely at address zero > > In practice, this is not possible as it represent counts of link maps. > Link maps have sizes larger than 1 byte, so the sum of any two link map > counts will always fit within a size_t without wrapping around. > > This patch therefore adds a check on remaining_to_add == 0 and tell GCC > that this can not happen using __builtin_unreachable. > > Thanks to Andreas Schwab for the investigation. > > Closes: BZ #32245 > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > --- > elf/dl-find_object.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/elf/dl-find_object.c b/elf/dl-find_object.c > index 449302eda3..ae18b438d3 100644 > --- a/elf/dl-find_object.c > +++ b/elf/dl-find_object.c > @@ -662,6 +662,14 @@ _dl_find_object_update_1 (struct link_map **loaded, size_t count) > = _dlfo_loaded_mappings[!active_idx]; > size_t remaining_to_add = current_used + count; > > + /* remaining_to_add can be 0 if (current_used + count) wraps, but in practice > + this is not possible as it represent counts of link maps. Link maps have > + sizes larger than 1 byte, so the sum of any two link map counts will > + always fit within a size_t without wrapping around. This check ensures > + that target_seg is not erroneously considered potentially NULL by GCC. */ > + if (remaining_to_add == 0) > + __builtin_unreachable (); > + > /* Ensure that the new segment chain has enough space. */ > { > size_t new_allocated Okay from me as well. Reviewed-by: Florian Weimer <fweimer@redhat.com> Thanks, Florian
Since this error had been blocking moving build-many-glibcs.py to defaulting to GCC 14, I tested whether we now had a clean build-many-glibcs.py with GCC 14 branch and so could switch the default. Unfortunately it seems we can't switch the default to GCC 14 branch yet because there's another failure there: GCC bug 103370, assembler errors building iso-2022-jp.c for ColdFire soft-float (a bug which has come and gone several times as GCC code generation changes, but is probably actually an m68k back-end bug). (Note that this is probably different from GCC bug 116244, the reload ICE building libstdc++ for ColdFire, both hard and soft float, with GCC mainline.) So for now we remain with GCC 13 branch as the build-many-glibcs.py default, as the most recent version to give a clean build across all configurations.
diff --git a/elf/dl-find_object.c b/elf/dl-find_object.c index 449302eda3..ae18b438d3 100644 --- a/elf/dl-find_object.c +++ b/elf/dl-find_object.c @@ -662,6 +662,14 @@ _dl_find_object_update_1 (struct link_map **loaded, size_t count) = _dlfo_loaded_mappings[!active_idx]; size_t remaining_to_add = current_used + count; + /* remaining_to_add can be 0 if (current_used + count) wraps, but in practice + this is not possible as it represent counts of link maps. Link maps have + sizes larger than 1 byte, so the sum of any two link map counts will + always fit within a size_t without wrapping around. This check ensures + that target_seg is not erroneously considered potentially NULL by GCC. */ + if (remaining_to_add == 0) + __builtin_unreachable (); + /* Ensure that the new segment chain has enough space. */ { size_t new_allocated
The remaining_to_add variable can be 0 if (current_used + count) wraps, This is caught by GCC 14+ on hppa, which determines from there that target_seg could be be NULL when remaining_to_add is zero, which in turns causes a -Wstringop-overflow warning: In file included from ../include/atomic.h:49, from dl-find_object.c:20: In function '_dlfo_update_init_seg', inlined from '_dl_find_object_update_1' at dl-find_object.c:689:30, inlined from '_dl_find_object_update' at dl-find_object.c:805:13: ../sysdeps/unix/sysv/linux/hppa/atomic-machine.h:44:4: error: '__atomic_store_4' writing 4 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=] 44 | __atomic_store_n ((mem), (val), __ATOMIC_RELAXED); \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ dl-find_object.c:644:3: note: in expansion of macro 'atomic_store_relaxed' 644 | atomic_store_relaxed (&seg->size, new_seg_size); | ^~~~~~~~~~~~~~~~~~~~ In function '_dl_find_object_update': cc1: note: destination object is likely at address zero In practice, this is not possible as it represent counts of link maps. Link maps have sizes larger than 1 byte, so the sum of any two link map counts will always fit within a size_t without wrapping around. This patch therefore adds a check on remaining_to_add == 0 and tell GCC that this can not happen using __builtin_unreachable. Thanks to Andreas Schwab for the investigation. Closes: BZ #32245 Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- elf/dl-find_object.c | 8 ++++++++ 1 file changed, 8 insertions(+)