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

login
register
mail settings
Submitter Chen Gang
Date Aug. 27, 2014, 10:43 p.m.
Message ID <53FE5EF6.5030003@gmail.com>
Download mbox | patch
Permalink /patch/383605/
State New
Headers show

Comments

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(-)
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

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;