On 09.08.2019 15:32, Christos Zoulas wrote:
> My worry is that someone will call pthread_setname_np() with a
> "%thread%s" name argument and get a core dump on a NetBSD system since
> the string will be interpreted as a format (where in other OS's it will
> be taken literally and work.
>
This will be caught by a compiler with __printflike() attribute.
Clang by default warns, GCC warns with -Wall.
We could make these kind of bugs error by default for the general benefit.
$ cat test.c
#include <stdio.h>
int
main(int argc, char **argv)
{
printf("%thread%s\n");
return 0;
}
$ clang test.c
test.c:6:12: warning: invalid conversion specifier 'h'
[-Wformat-invalid-specifier]
printf("%thread%s\n");
~~^
1 warning generated.
$ gcc test.c
$ gcc -Wall test.c
test.c: In function ‘main’:
test.c:6:12: warning: unknown conversion type character ‘h’ in format
[-Wformat=]
printf("%thread%s\n");
^
test.c:6:18: warning: format ‘%s’ expects a matching ‘char *’ argument
[-Wformat=]
printf("%thread%s\n");
~^
Sanitizers can catch it in runtime:
$ ./a.out
==22505==WARNING: unexpected format specifier in printf interceptor: %th
==22505==WARNING: unexpected format specifier in printf interceptor: %th
If this is a real concern, than probably better to switch to the Linux
API as is and patch all the users for NetBSD >= 9. However in my opinion
maintaining two fully incompatible variations from now for long time
might be too much burden.
> christos
>
>> On Aug 9, 2019, at 4:12 PM, Kamil Rytarowski <n54%gmx.com@localhost
>> <mailto:n54%gmx.com@localhost>> wrote:
>>
>> On 09.08.2019 14:42, Jaromír Doleček wrote:
>>> I'd change this immediatelly using the normal rewrite logic in headers
>>> we use for standard versioning. It would be really good to push this
>>> into netbsd-9 too.
>>>
>>
>> Ah right, we can do __rename() and do it now.
>>
>>>>>> Personally, I find it convenient to use it like
>>>>>> pthread_setname_np(t[i],
>>>>>> "thread %d, i) and I would like to keep using it.
>>>>>
>>>>> FWIW, I think that's a better suggestion than either of mine. I
>>>>> support that.
>>>>
>>>> Do we have to wait for an API bump for this? Seems pretty harmless.
>>>> Although
>>>> it is probably better to have:
>>>>
>>>> int pthread_setname_np(pthread_t, const char *);
>>>> and
>>>> int pthread_fmtname_np(pthread_t, const char * restrict format, ...)
>>>> __printflike(2, 3);
>>>>
>>>> christos
>>>>
>>
>> If we switch to pthread_setname_np() to fully API incompatible prototype
>> than we will need to reiterate over existing users and add #ifdef for in
>> my opinion too little gain.
>>
>> I did some patching and notifying (this happened in MESA) of 3rd party
>> code to support the NetBSD variation of pthread_setname_np(3) and I
>> don't want to reiterate over all the users now.
>>
>> My proposal is even (in practice) ABI compatible in a number of CPU ABIs
>> (such as i386/amd64)... however too risky to tell whether it applies to
>> all of them.
>>
>> I think it is enough to keep a single function to set thread name, no
>> need for two variations (setname and fmtname).
>>
>> There are around 30 patched programs in pkgsrc tweaking
>> pthread_setname_np() and a number of them with applied support upstream
>> (150 total? there are 144 references in Debian repositories).
>>
>> If someone will insist to use form pthread_setname_np(pthread_t, const
>> char *), it will be allowed to do it with the ... function type, just
>> pass full string in format and skip var args.
>>
>> <sanitizer.log>
>
Attachment:
signature.asc
Description: OpenPGP digital signature