* [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).