On 07.02.2020 23:13, Christos Zoulas wrote: > In article <CACQjD2FQ=7ABLqxrF7GrO8GvRm03OE55G_mRe4zAM4080h_Kiw%mail.gmail.com@localhost>, > Santhosh Raju <fox%netbsd.org@localhost> wrote: >> -=-=-=-=-=- >> >> Hello >> >> While working with kamil@ on MKLIBCSANITIZER based build, I came >> across a compile error in pam_krb5.c >> >> --- dependall-pam_krb5 --- >> /home/fox/projects/netbsd/obj-wip/destdir.amd64/usr/include/security/pam_modules.h: >> In function 'pam_sm_setcred': >> /home/fox/projects/netbsd/src-wip/lib/libpam/modules/pam_krb5/pam_krb5.c:489:6: >> error: null destination pointer [-Werror=format-overflow=] >> sprintf(p, "%d", getpid()); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> /home/fox/projects/netbsd/src-wip/lib/libpam/modules/pam_krb5/pam_krb5.c:485:6: >> error: null destination pointer [-Werror=format-overflow=] >> sprintf(p, "%d", pwd->pw_uid); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> This error is reproducible in gcc version 8.3.0, but not in gcc >> version 5.5.0 in NetBSD 8.1/amd4 (my build / test environment). It >> was also reproducible under gcc version 7.4.0 in Ubuntu (thanks to >> help from Ayushi who helped me out in testing the scenario there). >> >> After some contemplation on how to resolve this issue, I decided to >> replace sprintf(3) with snprinft(3) since this would just truncate the >> output in case of an overflow. I have attached a patch showing the >> changes. >> >> The "n" for the snprintf(3) has been chosen as PATH_MAX + 16 since >> this is the argument passed into to the calloc(3) when allocating >> space for "p". In addition to this (not shown in the patch), I think >> we should also keep track of the remaining buffer size left after each >> snprintf(3) invocation and break out of the loop when buffer size >> becomes less than 1 to prevent a buffer overflow. >> >> for example >> >> buffer_size = PATH_MAX + 16 >> >> ... >> >> buffer_size -= strlen(p) >> p += strlen(p) >> >> Since this change is in one of the userland tools which is an >> integral part in the NetBSD src tree, I seek advice on this change and it's >> potential implications and if this is the right way to solve it. The >> other option would be to add a -Wno-error=format-overflow and leave >> the code as is. >> >> On a side note, this error happens when the optimization flag is set >> to -O2 and the compile goes through when -O0 is passed in as the flag. > > Thanks, should be fixed now. > > christos > Thanks. Can we remove the '/* XXX potential overflow */' comment now?
Attachment:
signature.asc
Description: OpenPGP digital signature