Skip to content

Ignore locale creation failure #18337

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

Closed
wants to merge 1 commit into from
Closed

Conversation

t8m
Copy link
Member

@t8m t8m commented May 18, 2022

It can fail on Windows 7 for example and we have the fallback
to strcasecmp in place.

Fixes #18322

It can fail on Windows 7 for example and we have the fallback
to strcasecmp in place.

Fixes openssl#18322
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels May 18, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 18, 2022
@paulidale
Copy link
Contributor

I think we should implement our own strcasecmp. This is far too much like whack-a-mole.
Sure, this is the last thing we know about but they keep coming....

@bagder
Copy link

bagder commented May 18, 2022

That would be so much easier, less error prone and less complicated than this race you are on now. We did it in libcurl in 2008. Never had a problem, never looked back.

@paulidale
Copy link
Contributor

The only concern is performance -- strcasecmp has appeared on profiling output.

I still think we should do it.

@beldmit
Copy link
Member

beldmit commented May 18, 2022

I think it's sort of overkill. And we already have performance impact from the homebrew toupper/tolower implementations.

I don't like this fallback but I'm afraid it's inevitable. Is it possible to test your solution on Windows 7 with Turkish locale?

@bagder
Copy link

bagder commented May 18, 2022

they are also slow because they have all that extra locale knowledge you don't want, that also goes for tolower/toupper that you also can replace with your own versions that wouldn't care about locales

@paulidale
Copy link
Contributor

The strcasecmp problem is the gift that keeps on giving. We've had way too many problems with the current fix(es) and they keep on coming -- I do not believe it is worthwhile continuing with this ongoing source of pain. I'm open to someone else picking up the problem and guaranteeing to fix things in a timely manner, however I'm no longer confident it is possible.

@t8m
Copy link
Member Author

t8m commented May 18, 2022

My opinion is we should keep fixing things as they come and in the meanwhile someone can work on our homebrew strcasecmp implementation if they wish.

There is also the OPENSSL_NO_LOCALE define to workaround issues on old systems.

@t8m
Copy link
Member Author

t8m commented May 18, 2022

Btw I tried a naive strcasecmp implementation and it is orders of magnitude worse in performance than the glibc implementation even if I set a non C locale for the glibc implementation to try to make its job harder. :(

@bagder
Copy link

bagder commented May 18, 2022

if I set a non C locale

So again, why would you ever want locale-dependent string comparisons in a TLS library?

@t8m
Copy link
Member Author

t8m commented May 18, 2022

So again, why would you ever want locale-dependent string comparisons in a TLS library?

You would never want it BUT strcasecmp in glibc IS locale dependent unless you use strcasecmp_l(). And that is the problem!

@bagder
Copy link

bagder commented May 18, 2022

But you don't want that, so your own implementation would not need locale knowledge so comparing performance with that is pretty... well, off-topic.

@paulidale
Copy link
Contributor

The glibc implementation is pretty basic: https://code.woboq.org/userspace/glibc/string/strcasecmp.c.html
The tolower doesn't seem to be particularly optimised.

We'll have more overheads ...

Also POSIX says behaviour of these functions is undefined:

In the POSIX locale, strcasecmp() and strncasecmp() shall behave as if the strings had been converted to lowercase and then a byte comparison performed. The results are unspecified in other locales.

Undefined behaviour is awesome: https://twitter.com/m13253/status/1371615680068526081?lang=en

@t8m
Copy link
Member Author

t8m commented May 18, 2022

But you don't want that, so your own implementation would not need locale knowledge so comparing performance with that is pretty... well, off-topic.

Not at all, I've just set it for the glibc so it has to work harder. EVEN the a naive C implementation compiled with -O2 with fairly new gcc produces code that is orders of magnitude slower than glibc.

@t8m
Copy link
Member Author

t8m commented May 18, 2022

The glibc implementation is pretty basic: https://code.woboq.org/userspace/glibc/string/strcasecmp.c.html
The tolower doesn't seem to be particularly optimised.

I suppose there are more implementations in assembly code.

@t8m
Copy link
Member Author

t8m commented May 18, 2022

This is the difference in performance on my machine:

$ time ./testcasecmp kaboomi kaBoomI

real	0m0.260s
user	0m0.259s
sys	0m0.001s
$ time ./testmycasecmp kaboomi kaBoomI

real	0m6.314s
user	0m6.305s
sys	0m0.002s

@bagder
Copy link

bagder commented May 18, 2022

You prefer a seriously complicated solution using global variables that break on platforms you don't even test, just because your first naive implementation is slower than glibc? Roger that. I will exit from where I came from.

@piru
Copy link

piru commented May 18, 2022

This is just insane. Surely there are worse bottlenecks to worry about than string comparison?

@t8m
Copy link
Member Author

t8m commented May 18, 2022

The first naive implementation is something like https://code.woboq.org/userspace/glibc/string/strcasecmp.c.html

Even if I make it exactly the same, it is not much faster.

The only way to get better performance (comparable to glibc) is to do some serious assembler code hacking.

@piru, look at how strcasecmp is extensively used throughout the OpenSSL code - especially for OSSL_PARAM matching.

@t8m
Copy link
Member Author

t8m commented May 18, 2022

I am off this flamewar for now.

@t8m
Copy link
Member Author

t8m commented May 18, 2022

BTW @paulidale - to be super-safe we could instead of fallbacking to strcasecmp(), if the locale initialization fails, fall back to our own implementation.

@piru
Copy link

piru commented May 18, 2022

My parting comment: Has anyone actually measured the practical impact of this optimization has on real life application vs synthetic benchmarks?

This is just stupid on every level. I'm out.

@beldmit
Copy link
Member

beldmit commented May 18, 2022

My position is quite simple. We have a straightforward API on "new" systems (POSIX 2008, in fact) and should use this API there. We can use any fallbacks on elder systems if necessary, but only if necessary.

@paulidale
Copy link
Contributor

We have seen strcasecmp appear in profiling output, it's not insignificant. #16791 indicated 6-7% for ctype et al in the specific test case.

@paulidale
Copy link
Contributor

Closing in favour of #18344

@paulidale paulidale closed this May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: review pending This pull request needs review by a committer branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants