* [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border
@ 2014-08-27 22:43 Chen Gang
2014-08-27 22:51 ` Konstantin Serebryany
2014-09-01 8:42 ` Jakub Jelinek
0 siblings, 2 replies; 6+ messages in thread
From: Chen Gang @ 2014-08-27 22:43 UTC (permalink / raw)
To: jakub, dodji, kcc, dvyukov; +Cc: gcc-patches List, Jeff Law
'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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border
2014-08-27 22:43 [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border Chen Gang
@ 2014-08-27 22:51 ` Konstantin Serebryany
2014-08-27 22:58 ` Chen Gang
2014-09-01 8:42 ` Jakub Jelinek
1 sibling, 1 reply; 6+ messages in thread
From: Konstantin Serebryany @ 2014-08-27 22:51 UTC (permalink / raw)
To: Chen Gang
Cc: Jakub Jelinek, Dodji Seketeli, Kostya Serebryany, Dmitry Vyukov,
gcc-patches List, Jeff Law
[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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border
2014-08-27 22:51 ` Konstantin Serebryany
@ 2014-08-27 22:58 ` Chen Gang
2014-08-30 3:53 ` Chen Gang
0 siblings, 1 reply; 6+ messages in thread
From: Chen Gang @ 2014-08-27 22:58 UTC (permalink / raw)
To: Konstantin Serebryany
Cc: Jakub Jelinek, Dodji Seketeli, Kostya Serebryany, Dmitry Vyukov,
gcc-patches List, Jeff Law
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
Open share and attitude like air water and life which God blessed
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border
2014-08-27 22:58 ` Chen Gang
@ 2014-08-30 3:53 ` Chen Gang
0 siblings, 0 replies; 6+ messages in thread
From: Chen Gang @ 2014-08-30 3:53 UTC (permalink / raw)
To: Konstantin Serebryany
Cc: Jakub Jelinek, Dodji Seketeli, Kostya Serebryany, Dmitry Vyukov,
gcc-patches List, Jeff Law
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.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border
2014-08-27 22:43 [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border Chen Gang
2014-08-27 22:51 ` Konstantin Serebryany
@ 2014-09-01 8:42 ` Jakub Jelinek
2014-09-01 8:52 ` Chen Gang
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2014-09-01 8:42 UTC (permalink / raw)
To: Chen Gang; +Cc: dodji, kcc, dvyukov, gcc-patches List, Jeff Law
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border
2014-09-01 8:42 ` Jakub Jelinek
@ 2014-09-01 8:52 ` Chen Gang
0 siblings, 0 replies; 6+ messages in thread
From: Chen Gang @ 2014-09-01 8:52 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: dodji, kcc, dvyukov, gcc-patches List, Jeff Law
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.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-01 8:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-27 22:43 [PATCH] libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc: Avoid writing '\0' out of string's border Chen Gang
2014-08-27 22:51 ` Konstantin Serebryany
2014-08-27 22:58 ` Chen Gang
2014-08-30 3:53 ` Chen Gang
2014-09-01 8:42 ` Jakub Jelinek
2014-09-01 8:52 ` Chen Gang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).