-
Notifications
You must be signed in to change notification settings - Fork 626
Add new function ares_strnlen (#929) #931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add a new function `ares_strnlen()` to fix the logic in default_asetsockopt(), see <c-ares#929 (comment)>.
I think we should check to see if the system support strnlen() natively first, as it likely is optimized if it does, then fallback to this code if not. |
I prefer the easiest path which is this, and I won't be doing what you're suggesting to spare myself the time... it can always be implemented later too. |
Eh, the current cmake and autotools code is pretty straight forward to add something like that at least. It used to be the autotools code was almost impossible to update and get right but I fixed that earlier this year. The link you provided is kind of an odd thing for them to even mention. I'd expect most implementations don't handle pointer overflows without crashing, and that's likely ok as the caller will probably crash somewhere else due to such poor use. |
in src/lib/ares_config.h.cmake you also need to add
Unlike autotools, it doesn't auto-add stuff to the header. The last thing is we support some windows makefiles that don't use autotools or cmake, so src/lib/config-win32.h is used in those situations, and it looks like Windows does have strnlen, so something like this should be added to that file:
|
It should be possible to write a CMake function |
I honestly can't say I've seen that done, but I'm definitely no CMake expert. It would make it easier to maintain in the long run, obviously. |
Fixes an issue where Open Watcom does not have strnlen but has strnlen_s.
This reverts commit af0b53b.
@bradh352 I'm wondering if, in absence of |
@createyourpersonalaccount that reference doesn't really talk about strnlen_s just the _s functions in general. I'm good going the memchr() route, does sound easier / more portable. |
This reverts commit 15a8cf7.
I'm guessing EPEL must have just moved CentOS 7 support to archive ... guess I need to look at fixing the repo paths for that. Otherwise, this looks good to me, you ready for it to be merged? |
Yes I think it's fine. |
Add a new function `ares_strnlen()` to fix the logic in default_asetsockopt(), see <#929 (comment)>. Authored-By: Nikolaos Chatzikonstantinou (@createyourpersonalaccount)
Add a new function
ares_strnlen()
to fix the logic in default_asetsockopt(), see#929 (comment).