| Message ID | 53FE5EF6.5030003@gmail.com |
|---|---|
| State | New |
| Headers | show |
[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
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
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.
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
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.
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;