-
Notifications
You must be signed in to change notification settings - Fork 323
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
Fixed some issues found by tests with enabled UndefinedBehaviorSanitizer #1109
Conversation
For 505e879 ("Fixed undefined behaviour in left shift of int value")
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: See below comment...
Are you saying that if mmaps->size == 0 then mmaps->elts is NULL?
If so, I'd make the check (mmaps->elts == NULL)
For e059674 ("Initialize port_impl only when it is needed")
|
src/nxt_http_request.c
Outdated
if (nxt_slow_path(r->args->length == 0)) { | ||
r->args_decoded.length = 0; | ||
goto end; | ||
} | ||
|
||
start = r->args->start; | ||
end = start + r->args->length; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if this is because r->args->start can be NULL, then I'd check for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in less code, and as @ac000 says, is more clear about why we're doing that:
start = r->args->start ?: (u_char) "";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll short circuit out the for () loop the only question is do we want to (or not) do the bit of code between the for () loop and the end label in such situations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short circuiting optimizes for the case where .length == 0, at the cost of adding some more code (possibly pessimizing the rest of the cases by one op or so, and also (subjectively) pessimizing readability). I guess it depends on how likely each of the cases are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't clear, the short circuiting is fine. But by not just going to end:
when ->start is NULL means we'll execute some other stuff that we may not want to...
Looking closer, assuming the dst = dst_start
in the for loop initialiser happens, then we will execute
r->args_decoded.length = dst - r->args_decoded.start;
Which will equate to dst - dst
which should be 0
and
if (name_length != 0 || dst != dst_start) {
wil equate to false. So I think those are OK.
So yes, the ternary should also do the trick...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if this is because r->args->start can be NULL, then I'd check for that.
It can.
For example:
/foo: r->args->start is NULL
/foo?: r->args->start is not NULL
And for the configuration:
{
"match": {
"query": ""
}
}
The pattern related to query
is also NULL.
pattern->start = NULL;
So it looks like we don't need to touch r->args_decoded
if there is no UB reported. Otherwise, not sure if we'll have to rethink the parrten->start to query "" value.
I'd would suggest fixing the case of r->args_start
is NULL. For example:
if (nxt_slow_path(r->args->start == NULL)) {
goto end;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me of https://github.com/mkirchner/linked-list-good-taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I actually have that email from Linus saved off for whenever I need to refer to it...
I think what Alex is saying is that with his ternary suggestion the code just flows through and does the right thing ragardless if r->args->start
is NULL
or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it's the intention of the current code.
The fixing is to make the tool that reported UB happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with Alex's suggestion and fix it like you did in NJS: avoiding arithmetic ops with NULL pointer in r->args
eabd7c7
to
ca37cf1
Compare
Replaced length vs 0 checks by string vs NULL where requested.
|
If you could add my |
ca37cf1
to
da0802e
Compare
Rebased and added
|
da0802e
to
b82297c
Compare
Rebased, two more
|
b82297c
to
8eb3653
Compare
|
Otherwise, undefined behaviour will be triggered. Can be reproduced by test/test_routing.py::test_routes_match_host_empty with enabled UndefinedBehaviorSanitizer: src/nxt_http_route.c:2141:17: runtime error: applying zero offset to null pointer #0 0x100562588 in nxt_http_route_test_rule nxt_http_route.c:2091 nginx#1 0x100564ed8 in nxt_http_route_handler nxt_http_route.c:1574 nginx#2 0x10055188c in nxt_http_request_action nxt_http_request.c:570 nginx#3 0x10052b1a0 in nxt_h1p_request_body_read nxt_h1proto.c:998 nginx#4 0x100449c38 in nxt_event_engine_start nxt_event_engine.c:542 nginx#5 0x100436828 in nxt_thread_trampoline nxt_thread.c:126 nginx#6 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030) nginx#7 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_route.c:2141:17 Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Can be reproduced by test/test_rewrite.py::test_rewrite_njs with enabled UndefinedBehaviorSanitizer: src/nxt_http_js.c:169:52: runtime error: applying zero offset to null pointer #0 0x10255b044 in nxt_http_js_ext_get_args nxt_http_js.c:169 nginx#1 0x102598ad0 in njs_value_property njs_value.c:1175 nginx#2 0x10259c2c8 in njs_vm_object_prop njs_vm.c:1398 nginx#3 0x102559d74 in nxt_js_call nxt_js.c:445 nginx#4 0x1023c0da0 in nxt_tstr_query nxt_tstr.c:276 nginx#5 0x102516ec4 in nxt_http_rewrite nxt_http_rewrite.c:56 nginx#6 0x1024fd86c in nxt_http_request_action nxt_http_request.c:565 nginx#7 0x1024d71b0 in nxt_h1p_request_body_read nxt_h1proto.c:998 nginx#8 0x1023f5c48 in nxt_event_engine_start nxt_event_engine.c:542 nginx#9 0x1023e2838 in nxt_thread_trampoline nxt_thread.c:126 nginx#10 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030) nginx#11 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52 Same fix was introduced in NJS: <http://hg.nginx.org/njs/rev/4fba78789fe4> Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Found by UndefinedBehaviorSanitizer: src/nxt_random.c:151:31: runtime error: left shift of 140 by 24 places cannot be represented in type 'int' #0 0x104f78968 in nxt_random nxt_random.c:151 nginx#1 0x104f58a98 in nxt_shm_open nxt_port_memory.c:377 nginx#2 0x10503e24c in nxt_controller_conf_send nxt_controller.c:617 nginx#3 0x105041154 in nxt_controller_process_request nxt_controller.c:1109 nginx#4 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542 nginx#5 0x104f27254 in main nxt_main.c:35 nginx#6 0x180fbd0dc (<unknown module>) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_random.c:151:31 Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Can be reproduced by test/test_variables.py::test_variables_dynamic_arguments with enabled UndefinedBehaviorSanitizer: src/nxt_http_request.c:961:17: runtime error: applying zero offset to null pointer #0 0x1050d95a4 in nxt_http_arguments_parse nxt_http_request.c:961 nginx#1 0x105102bf8 in nxt_http_var_arg nxt_http_variables.c:621 nginx#2 0x104f95d74 in nxt_var_interpreter nxt_var.c:507 nginx#3 0x104f98c98 in nxt_tstr_query nxt_tstr.c:265 nginx#4 0x1050abfd8 in nxt_router_access_log_writer nxt_router_access_log.c:194 nginx#5 0x1050d81f4 in nxt_http_request_close_handler nxt_http_request.c:838 nginx#6 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542 nginx#7 0x104fba838 in nxt_thread_trampoline nxt_thread.c:126 nginx#8 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030) nginx#9 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_request.c:961:17 Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Can be reproduced by test/test_settings.py::test_settings_send_timeout with enabled UndefinedBehaviorSanitizer. Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Found by UndefinedBehaviorSanitizer. Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Found by UndefinedBehaviorSanitizer. Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
8eb3653
to
887fa46
Compare
|
src/nxt_http_js.c
Outdated
@@ -152,6 +152,8 @@ static njs_int_t | |||
nxt_http_js_ext_get_args(njs_vm_t *vm, njs_object_prop_t *prop, | |||
njs_value_t *value, njs_value_t *setval, njs_value_t *retval) | |||
{ | |||
u_char *start; | |||
uint8_t length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe size_t
is better.
src/nxt_http_request.c
Outdated
if (nxt_slow_path(r->args->length == 0)) { | ||
r->args_decoded.length = 0; | ||
goto end; | ||
} | ||
|
||
start = r->args->start; | ||
end = start + r->args->length; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it's the intention of the current code.
The fixing is to make the tool that reported UB happy.
No description provided.