Modified OSSL_parse_url to force initialization of pport_num to 0. #21109
Conversation
This change is intended to provide some safety for uninitialized stack failures that have appeared in 80-test_cmp_http on NonStop x86 when run in a complex CI/CD Jenkins environment. This change also adds init_pint() to handle the initialization of a pointer to int value. Fixes: openssl#21083 Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca>
|
This PR is based on 3.1, tested with 3.1 and master, and can also be applied to master, and 3.0 as well. |
paulidale
added
branch: master
levitte
added
approval: done
| @@ -54,6 +61,7 @@ int OSSL_parse_url(const char *url, char **pscheme, char **puser, char **phost, | |||
| init_pstring(puser); | |||
| init_pstring(phost); | |||
| init_pstring(pport); | |||
| init_pint(pport_num); | |||
There was a problem hiding this comment.
I really do not understand how this change can help. Without it, the *pport_num will be left uninitialized only in case the OSSL_parse_url() call fails. Why that could change anything? IMO this is not the cause for the problem you've seen @rsbeckerca. I suspect there is something more subtle with these tests in regards to timing, etc.
There was a problem hiding this comment.
The line:
openssl/crypto/http/http_lib.c
Line 113 in 168d93a
does not use
sscanf correctly. Specifying !scanf strictly checks for 0 vs. non-zero instead of what would be semantically better as sscanf(...) != 1, since sscanf returns the number of arguments processed rather than an error. I understand that that check is probably fine. Also, there is a redundant initialization of pport in OSSL_HTTP_parse_url.
I am just trying to be safe about initialized values on the stack. If the PR does not matter, it can be dropped.
There was a problem hiding this comment.
Good point that the result of sscanf() should better be checked using != 1.
Yet in this case with one variable to be filled, (effectively) using == 0 boils down to the same.
There was a problem hiding this comment.
The call init_pstring(pport); in OSSL_HTTP_parse_url() is not redundant -
note that the subsequent
if (!OSSL_parse_url(url, &scheme, puser, phost, &port, pport_num,
ppath, pquery, pfrag))
return 0;
uses a different port variable pointer (&port) and may return on error, and the initialization is for that case.
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
|
CIFuzz is failing, and if this is going into master, shouldn't this PR be against the master branch? |
It is not required. CIFuzz fail is unrelated. |
|
Merged to master, 3.1, and 3.0 branches. Thank you. |
This change is intended to provide some safety for uninitialized stack failures that have appeared in 80-test_cmp_http on NonStop x86 when run in a complex CI/CD Jenkins environment. This change also adds init_pint() to handle the initialization of a pointer to int value. Fixes: #21083 Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21109) (cherry picked from commit 45cd255)
This change is intended to provide some safety for uninitialized stack failures that have appeared in 80-test_cmp_http on NonStop x86 when run in a complex CI/CD Jenkins environment. This change also adds init_pint() to handle the initialization of a pointer to int value. Fixes: #21083 Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21109)
This change is intended to provide some safety for uninitialized stack failures that have appeared in 80-test_cmp_http on NonStop x86 when run in a complex CI/CD Jenkins environment. This change also adds init_pint() to handle the initialization of a pointer to int value. Fixes: #21083 Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21109) (cherry picked from commit 45cd255)
This change is intended to provide some safety for occasional random failures that have appeared in 80-test_cmp_http
on NonStop x86 when run in a complex CI/CD Jenkins environment. This change also adds init_pint() to handle the
initialization of a pointer to int value.
Fixes #21083