8000 Add new function ares_strnlen (#929) by createyourpersonalaccount · Pull Request #931 · c-ares/c-ares · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 11 commits into from
Dec 11, 2024

Conversation

createyourpersonalaccount
Copy link
Contributor

Add a new function ares_strnlen() to fix the logic in default_asetsockopt(), see
#929 (comment).

Add a new function `ares_strnlen()` to fix the logic in
default_asetsockopt(), see
<c-ares#929 (comment)>.
@bradh352
Copy link
Member
bradh352 commented Dec 8, 2024

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.

@createyourpersonalaccount
Copy link
Contributor Author

@bradh352

  1. Needs to be done for both autotools & CMake (fun!)
  2. Need to guarantee same semantics across all implementations (even more fun!) (e.g. this)

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.

@bradh352
Copy link
Member
bradh352 commented Dec 8, 2024

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.

@bradh352
Copy link
Member
bradh352 commented Dec 9, 2024

in src/lib/ares_config.h.cmake you also need to add

/* Define to 1 if you have the strnlen function. */
#cmakedefine HAVE_STRNLEN 1

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:

/* Define if you have the strnlen function. */
#define HAVE_STRNLEN 1

@createyourpersonalaccount
Copy link
Contributor Author
createyourpersonalaccount commented Dec 9, 2024

Unlike autotools, it doesn't auto-add stuff to the header.

It should be possible to write a CMake function MY_CHECK_SYMBOL_EXISTS() to auto-add those defines. Do you want that?

@bradh352
Copy link
Member
bradh352 commented Dec 9, 2024

Unlike autotools, it doesn't auto-add stuff to the header.

It should be possible to write a CMake function MY_CHECK_SYMBOL_EXISTS() to auto-add those defines. Do you want that?

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.
@createyourpersonalaccount
Copy link
Contributor Author

@bradh352
Here's info on strnlen_s, https://stackoverflow.com/a/50921096. Let me draw your attention to the fact that strnlen_s is, according to him, not supported by Windows as defined in the C standard annex.

I'm wondering if, in absence of strnlen, we should just fall back to memchr(s, 0, maxlen). Or perhaps indeed scrap the entire PR and just use memchr.

@bradh352
Copy link
Member

@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.

@bradh352
Copy link
Member

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?

@createyourpersonalaccount
Copy link
Contributor Author

@bradh352

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.

@bradh352 bradh352 merged commit d52e3eb into c-ares:main Dec 11, 2024
26 of 27 checks passed
bradh352 pushed a commit that referenced this pull request Dec 11, 2024
Add a new function `ares_strnlen()` to fix the logic in
default_asetsockopt(), see
<#929 (comment)>.

Authored-By: Nikolaos Chatzikonstantinou (@createyourpersonalaccount)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0