Skip to content
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

locatetest doesn't seem to test properly #18205

Closed
paulidale opened this issue Apr 29, 2022 · 5 comments
Closed

locatetest doesn't seem to test properly #18205

paulidale opened this issue Apr 29, 2022 · 5 comments
Assignees
Labels
triaged: bug The issue/pr is/fixes a bug

Comments

@paulidale
Copy link
Contributor

paulidale commented Apr 29, 2022

Refer:

openssl/test/localetest.c

Lines 94 to 103 in bbe909d

TEST_ptr(setlocale(LC_ALL, ""));
res = strcasecmp(str1, str2);
TEST_note("Case-insensitive comparison via strcasecmp in current locale %s\n", res ? "failed" : "succeeded");
TEST_false(OPENSSL_strcasecmp(str1, str2));
cert = d2i_X509(NULL, &p, sizeof(der_bytes));
if (!TEST_ptr(cert))
return 0;

Should the TEST_ptr on line 94 and the TEST_false on line 99 both be wrapped by if (...) return 0;?
Without this, the test won't fail but will log output with ERROR in it. IMO, this is not a good thing.

@paulidale paulidale added the triaged: question The issue contains a question label Apr 29, 2022
@beldmit
Copy link
Member

beldmit commented Apr 29, 2022

If we failed on line 94, all other tests don't have much sense and probably we should skip it. You are definitely correct about line 99, if we failed, we should return an error immediately.

@paulidale
Copy link
Contributor Author

Maybe line 94 should become:

if (setlocale(LC_ALL, "") == NULL)
    return TEST_skip(...);

You could then drop the TEST_note().

@paulidale paulidale added triaged: bug The issue/pr is/fixes a bug and removed triaged: question The issue contains a question labels Apr 29, 2022
@beldmit
Copy link
Member

beldmit commented Apr 29, 2022

TEST_note has an intention to show if strcasecmp gives an expected result (yes for C, no for Turkish)

@paulidale
Copy link
Contributor Author

The skip message indicates a bad result. The test continuing indicates a good one. It isn't providing any new information and it clutters the test output (which is already too verbose).

@beldmit
Copy link
Member

beldmit commented Apr 29, 2022

It's a different information. If strcasecmp works properly, there should be no problems (or crash) in OPENSSL_strcasecmp. Otherwise it's crucial for us that OPENSSL_strcasecmp works properly when strcasecmp` does not.

@beldmit beldmit mentioned this issue Apr 29, 2022
2 tasks
beldmit added a commit to beldmit/openssl that referenced this issue Apr 29, 2022
openssl-machine pushed a commit that referenced this issue May 2, 2022
Fixes #18205

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18211)

(cherry picked from commit 93983e5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants