diff mbox series

adjust thread db function declarations to match definitions (BZ 26686)

Message ID 491dac14-811a-2d17-1425-d8f400262af7@gmail.com
State Committed
Commit 3eff7504cab0c406dbd27a1b07a413dafc39634d
Headers
Series adjust thread db function declarations to match definitions (BZ 26686) |

Commit Message

Martin Sebor Oct. 5, 2020, 9:43 p.m. UTC 
  Similar to the issue with the RPC function declarations, building
Glibc with the latest GCC 11 also shows a couple of instances of
the new -Warray-parameter warning in the thread db APIs.

To avoid these, the attached patch changes the deefinitions of
the two functions to match their definitions.

I tested the patch by building Glibc with GCC trunk and confirming
the warnings are gone, and by running the tests and confirming no
new failures in the test suite.

Martin

PS The functions only appear to access the first element of the array
(via the DB_DESC_SIZE() macro), so I at first thought an alternate
change might be to have both their declarations and definitions take
a uint32_t[1] instead.  But it turns out that they call
_td_locate_field with the array as an argument, and that function
accesses all three elements.  So declaring them to take uint32_t[1]
leads to warnings about _td_locate_field accessing the array past
its end.

PPS Another change to consider here is to also use the [static]
notation when declaring the arrays in internal functions (in
public headers it would require hiding the "static" behind a macro
to work with C++).  It's not necessary to enable bounds checking
with GCC 11 but it is with Clang.

Comments

Florian Weimer Oct. 6, 2020, 7:58 a.m. UTC | #1 
* Martin Sebor via Libc-alpha:

> Similar to the issue with the RPC function declarations, building
> Glibc with the latest GCC 11 also shows a couple of instances of
> the new -Warray-parameter warning in the thread db APIs.
>
> To avoid these, the attached patch changes the deefinitions of
> the two functions to match their definitions.
>
> I tested the patch by building Glibc with GCC trunk and confirming
> the warnings are gone, and by running the tests and confirming no
> new failures in the test suite.

Patch looks fine to me.

> PS The functions only appear to access the first element of the array
> (via the DB_DESC_SIZE() macro), so I at first thought an alternate
> change might be to have both their declarations and definitions take
> a uint32_t[1] instead.  But it turns out that they call
> _td_locate_field with the array as an argument, and that function
> accesses all three elements.  So declaring them to take uint32_t[1]
> leads to warnings about _td_locate_field accessing the array past
> its end.

Didn't we have a previous static analysis report that pointed to a
buffer overflow in these functions, and we couldn't figure out what was
going on because the actual allocation was large enough?  The paragraph
above certainly would explain this, assuming the other tool also took
the [2] array size to mean that only two elements can be accessed.

Thanks,
Florian
diff mbox series

Patch

diff --git a/nptl_db/fetch-value.c b/nptl_db/fetch-value.c
index 128d736adb..8f1ff74fd0 100644
--- a/nptl_db/fetch-value.c
+++ b/nptl_db/fetch-value.c
@@ -140,7 +140,7 @@  _td_fetch_value (td_thragent_t *ta,
 
 td_err_e
 _td_store_value (td_thragent_t *ta,
-		 uint32_t desc[2], int descriptor_name, psaddr_t idx,
+		 db_desc_t desc, int descriptor_name, psaddr_t idx,
 		 psaddr_t address, psaddr_t widened_value)
 {
   ps_err_e err;
@@ -240,7 +240,7 @@  _td_fetch_value_local (td_thragent_t *ta,
 
 td_err_e
 _td_store_value_local (td_thragent_t *ta,
-		       uint32_t desc[2], int descriptor_name, psaddr_t idx,
+		       db_desc_t desc, int descriptor_name, psaddr_t idx,
 		       void *address, psaddr_t widened_value)
 {
   td_err_e terr = _td_locate_field (ta, desc, descriptor_name, idx, &address);