Ensure indirect function calls have the correct type.#1994
Ensure indirect function calls have the correct type.#1994jkbonfield merged 1 commit intosamtools:developfrom
Conversation
There was a problem hiding this comment.
Looks OK and I can confirm the errors when built using clang21 -fsanitize=kcfi go away with this PR. (I'm assuming there are samtools and bcftools updates coming too.)
My only query is on the function naming.
Note: you also have a typo in your commit message.
"kgetline() to read lines from a FILE * to a kstring" should read
"kfgetline() to read lines from a FILE * to a kstring"
| EOF is returned at end of file or on error. | ||
| */ | ||
| HTSLIB_EXPORT | ||
| int hget_kstr(struct kstring_t *kstr, hFILE *fp) HTS_RESULT_USED; |
There was a problem hiding this comment.
Why hget_kstr and not khgetline?
We have a new kfgetline that works via fgets, so khgetline using hgets seems like the more memorable function name. Either that or rename 'kfgetline' to fget_kstr, but the getline ones seem more appropriate.
There was a problem hiding this comment.
The name was a modification of hgets() above, which itself derives from fgets(). kgetline() of course derived from the POSIX getline() function. I guess I could make it khgetline(), although having two functions that differ by one letter could be its own annoyance...
There was a problem hiding this comment.
Actually, the return value better matches that for getline(), so I'll change it.
Some HTSlib interfaces (notably kgetline(), kgetline2(), hts_itr_querys() and hts_parse_region()) have function callbacks that are intended to be generic, so the function signatures include a `void *` for data to be passed in. E.g. for kgetline() this would be the FILE * or hFILE * to read. Historically they have often been called with a function that has an explicit type (e.g. FILE * for fgets() used with kgetline) on the assumption that casting it to void * is harmless. Unfortunately doing this is strictly undefined behaviour and will fail in some conditions - for example if compiling with control flow integrity turned on. Recent versions of clang will also call this out when using undefined behaviour sanitizer. The solution is to create wrapper functions that do take a void *, and call the intended function with suitable casts. Some new interfaces are also added so that the messy implementation details can be hidden away, and the inputs can be better type checked. Hopefully this should also allow the wrapper functions to be inlined away. * khgetline() to read lines from an hFILE * to a kstring. * kfgetline() to read lines from a FILE * to a kstring. * tbx_itr_querys1() to create a tabix iterator on a region. Used to reimplement the existing tbx_itr_querys() macro. * bcf_itr_querys1() to create a region iterator an a bcf file Used to reimplement the existing bcf_itr_querys() macro.
daviesrob
force-pushed
the
function-pointers
branch
from
7c95acb to
b3faeb3
Compare
|
|
2 participants
Some HTSlib interfaces (notably
kgetline(),kgetline2(),hts_itr_querys()andhts_parse_region()) have function callbacks that are intended to be generic, so the function signatures include avoid *for data to be passed in. E.g. forkgetline()this would be theFILE *orhFILE *to read. Historically they have often been called with a function that has an explicit type (e.g.FILE *forfgets()used withkgetline) on the assumption that casting it tovoid *is harmless. Unfortunately doing this is strictly undefined behaviour and will fail in some conditions - for example if compiling with control flow integrity turned on. Recent versions of clang will also call this out when using undefined behaviour sanitizer. The solution is to create wrapper functions that do take avoid *, and call the intended function with suitable casts.Some new interfaces are also added so that the messy implementation details can be hidden away, and the inputs can be better type checked. Hopefully this should also allow the wrapper functions to be inlined away.
hget_kstr()to read lines from anhFILE *to akstring.kgetline()to read lines from aFILE *to akstring.tbx_itr_querys1()to create a tabix iterator on a region. Used to reimplement the existingtbx_itr_querys()macro.bcf_itr_querys1()to create a region iterator an a bcf file Used to reimplement the existingbcf_itr_querys()macro.