libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border

Submitted by Chen Gang on Aug. 27, 2014, 10:43 p.m.

Details

Message ID 53FE5EF6.5030003@gmail.com
State New
Headers show

Commit Message

Chen Gang Aug. 27, 2014, 10:43 p.m.
'max_len' is the maximized length of 'name', so for writing '\0' to
"name[max_len]", it is out of string's border, need use "max_len - 1"
instead of.

Pass normal test suite: "configure && make && make check && compare",
I guess, at present, it is not really used by outside, though.

2014-08-27  Chen Gang  <gang.chen.5i5j@gmail.com>

	* sanitizer_common/sanitizer_linux_libcdep.cc
	(SanitizerGetThreadName): Avoid writing '\0' out of string's
	border
---
 libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Konstantin Serebryany Aug. 27, 2014, 10:51 p.m.
[replying text only]

Hi Chen,
as per https://code.google.com/p/address-sanitizer/wiki/HowToContribute
all changes to libsanitizer, even such simple ones,
have to go through the LLVM tree first.

But, what makes you think there is a bug here?
The comment in sanitizer_common/sanitizer_common.h says:
// name should have space for at least max_len+1 bytes.

--kcc

On Wed, Aug 27, 2014 at 3:43 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> 'max_len' is the maximized length of 'name', so for writing '\0' to
> "name[max_len]", it is out of string's border, need use "max_len - 1"
> instead of.
>
> Pass normal test suite: "configure && make && make check && compare",
> I guess, at present, it is not really used by outside, though.
>
> 2014-08-27  Chen Gang  <gang.chen.5i5j@gmail.com>
>
>         * sanitizer_common/sanitizer_linux_libcdep.cc
>         (SanitizerGetThreadName): Avoid writing '\0' out of string's
>         border
> ---
>  libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> index e754b26..b9089d5 100644
> --- a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> +++ b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> @@ -140,7 +140,7 @@ bool SanitizerGetThreadName(char *name, int max_len) {
>    if (prctl(PR_GET_NAME, (unsigned long)buff, 0, 0, 0))  // NOLINT
>      return false;
>    internal_strncpy(name, buff, max_len);
> -  name[max_len] = 0;
> +  name[max_len - 1] = 0;
>    return true;
>  #else
>    return false;
> --
> 1.9.3
Chen Gang Aug. 27, 2014, 10:58 p.m.
On 08/28/2014 06:51 AM, Konstantin Serebryany wrote:
> [replying text only]
> 
> Hi Chen,
> as per https://code.google.com/p/address-sanitizer/wiki/HowToContribute
> all changes to libsanitizer, even such simple ones,
> have to go through the LLVM tree first.
> 

OK, thanks, I shall notice about it, next.

> But, what makes you think there is a bug here?
> The comment in sanitizer_common/sanitizer_common.h says:
> // name should have space for at least max_len+1 bytes.
> 

Oh, really, but for me, I still prefer to let max_len as all real buffer
length which like common sense (especially for extern functions).

If this extern function is not real used, at present (but will be used
next), for me, I still want to improve it (about max_len).


Thanks.

> --kcc
> 
> On Wed, Aug 27, 2014 at 3:43 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
>> 'max_len' is the maximized length of 'name', so for writing '\0' to
>> "name[max_len]", it is out of string's border, need use "max_len - 1"
>> instead of.
>>
>> Pass normal test suite: "configure && make && make check && compare",
>> I guess, at present, it is not really used by outside, though.
>>
>> 2014-08-27  Chen Gang  <gang.chen.5i5j@gmail.com>
>>
>>         * sanitizer_common/sanitizer_linux_libcdep.cc
>>         (SanitizerGetThreadName): Avoid writing '\0' out of string's
>>         border
>> ---
>>  libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>> index e754b26..b9089d5 100644
>> --- a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>> +++ b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>> @@ -140,7 +140,7 @@ bool SanitizerGetThreadName(char *name, int max_len) {
>>    if (prctl(PR_GET_NAME, (unsigned long)buff, 0, 0, 0))  // NOLINT
>>      return false;
>>    internal_strncpy(name, buff, max_len);
>> -  name[max_len] = 0;
>> +  name[max_len - 1] = 0;
>>    return true;
>>  #else
>>    return false;
>> --
>> 1.9.3
Chen Gang Aug. 30, 2014, 3:58 a.m.
On 8/28/14 6:58, Chen Gang wrote:
> On 08/28/2014 06:51 AM, Konstantin Serebryany wrote:
>> But, what makes you think there is a bug here?
>> The comment in sanitizer_common/sanitizer_common.h says:
>> // name should have space for at least max_len+1 bytes.
>>
> 
> Oh, really, but for me, I still prefer to let max_len as all real buffer
> length which like common sense (especially for extern functions).
> 
> If this extern function is not real used, at present (but will be used
> next), for me, I still want to improve it (about max_len).

In the current gcc source code, it is not used, but I guess, it may be
used, next.

Theoretically, we can treate all extern functions as API, which need be
more careful about its declarations (include parameters definition), or
may borther many callers:

 - If caller has duty to be sure of '\0' terminated (e.g. strncpy),
   callee need not care about it. For our case, need remove
   "name[max_len] = 0;".

 - If callee has duty to be sure of '\0' terminated (snprintf, gets),
   caller need not care about it. For our case, need use "max_len - 1"
   instead of "max_len".

For me, the extern function is neccesary to be improved in time (before
it is used by others). Or as an API, it is hard to be changed again.


Thanks.
Jakub Jelinek Sept. 1, 2014, 8:41 a.m.
On Thu, Aug 28, 2014 at 06:43:02AM +0800, Chen Gang wrote:
> 'max_len' is the maximized length of 'name', so for writing '\0' to
> "name[max_len]", it is out of string's border, need use "max_len - 1"
> instead of.

Depends on how the function's API is defined.
And, at least in GCC sources that function seems to be completely unused,
nothing calls it, so it is hard to guess what the API should be.

> 2014-08-27  Chen Gang  <gang.chen.5i5j@gmail.com>
> 
> 	* sanitizer_common/sanitizer_linux_libcdep.cc
> 	(SanitizerGetThreadName): Avoid writing '\0' out of string's
> 	border

	Jakub
Chen Gang Sept. 1, 2014, 8:57 a.m.
On 9/1/14 16:41, Jakub Jelinek wrote:
> On Thu, Aug 28, 2014 at 06:43:02AM +0800, Chen Gang wrote:
>> 'max_len' is the maximized length of 'name', so for writing '\0' to
>> "name[max_len]", it is out of string's border, need use "max_len - 1"
>> instead of.
> 
> Depends on how the function's API is defined.
> And, at least in GCC sources that function seems to be completely unused,
> nothing calls it, so it is hard to guess what the API should be.
> 

For me, if we are sure it is useless in future, we need remove it, now.

If we are sure it is useful in the future, we need improve it in time
(before it is used), it is not a good idea to let both caller and callee
notice about '\0':

 - If caller has duty to notice about '\0', callee need not notice about
   it: remove "name[max_len] = 0;"

 - If callee has duty to notice about '\0', caller need not notice about
   it: use "max_len - 1" instead of "max_len".

 - For both cases, the related comments of declaration are redundancy,
   need be removed (or improved).

And for safety (also easy understanding) reason, I prefer to remove it
firstly.


Thanks.

Patch hide | download patch | download mbox

diff --git a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
index e754b26..b9089d5 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
@@ -140,7 +140,7 @@  bool SanitizerGetThreadName(char *name, int max_len) {
   if (prctl(PR_GET_NAME, (unsigned long)buff, 0, 0, 0))  // NOLINT
     return false;
   internal_strncpy(name, buff, max_len);
-  name[max_len] = 0;
+  name[max_len - 1] = 0;
   return true;
 #else
   return false;