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

Undefined Behavior sanitizer: downcast is undefined behavior #26131

Closed
hashseed opened this issue Feb 15, 2019 · 5 comments
Closed

Undefined Behavior sanitizer: downcast is undefined behavior #26131

hashseed opened this issue Feb 15, 2019 · 5 comments

Comments

@hashseed
Copy link
Member

This is showing up when compiling with UBSan-Vptr:

[2019-02-15 15:01:34] yangguo@yangguo:~/node-ci/node-ci$ ninja -C out/Release/ gn_all
ninja: Entering directory `out/Release/'
[2112/2130] ACTION //node:generate_code_cache(//build/toolchain/linux:clang_x64)
FAILED: gen/node/node_code_cache.cc 
python ../../tools/generate_code_cache.py node_no_cache ../../node/tools/generate_code_cache.js gen/node/node_code_cache.cc
../../buildtools/third_party/libc++/trunk/include/memory:4946:33: runtime error: downcast of address 0x5621d59c3480 which does not point to an object of type 'node::options_parser::OptionsParser<node::EnvironmentOptions>::OptionField<bool>'
0x5621d59c3480: note: object is of type 'auto node::options_parser::OptionsParser<node::EnvironmentOptions>::Convert<node::options_parser::OptionsParser<node::DebugOptions>::OptionField<bool>, node::DebugOptions>(std::__1::shared_ptr<node::options_parser::OptionsParser<node::DebugOptions>::OptionField<bool> >, node::DebugOptions* (node::EnvironmentOptions::*)())::AdaptedField'
 00 00 00 00  f0 84 66 d3 21 56 00 00  e8 01 9c d5 21 56 00 00  d0 01 9c d5 21 56 00 00  20 d4 a8 d1
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'auto node::options_parser::OptionsParser<node::EnvironmentOptions>::Convert<node::options_parser::OptionsParser<node::DebugOptions>::OptionField<bool>, node::DebugOptions>(std::__1::shared_ptr<node::options_parser::OptionsParser<node::DebugOptions>::OptionField<bool> >, node::DebugOptions* (node::EnvironmentOptions::*)())::AdaptedField'
    #0 0x5621d1a9d50f in std::__1::enable_if<(!(is_array<node::options_parser::OptionsParser<node::EnvironmentOptions>::OptionField<bool> >::value)) && (!(is_array<node::options_parser::OptionsParser<node::EnvironmentOptions>::BaseOptionField>::value)), std::__1::shared_ptr<node::options_parser::OptionsParser<node::EnvironmentOptions>::OptionField<bool> > >::type std::__1::static_pointer_cast<node::options_parser::OptionsParser<node::EnvironmentOptions>::OptionField<bool>, node::options_parser::OptionsParser<node::EnvironmentOptions>::BaseOptionField>(std::__1::shared_ptr<node::options_parser::OptionsParser<node::EnvironmentOptions>::BaseOptionField> const&) buildtools/third_party/libc++/trunk/include/memory:4946:33
    #1 0x5621d1a9e22e in auto node::options_parser::OptionsParser<node::EnvironmentOptions>::Convert<node::DebugOptions>(node::options_parser::OptionsParser<node::DebugOptions>::Implication, node::DebugOptions* (node::EnvironmentOptions::*)()) node/src/node_options-inl.h:208:5
    #2 0x5621d1a8d254 in void node::options_parser::OptionsParser<node::EnvironmentOptions>::Insert<node::DebugOptions>(node::options_parser::OptionsParser<node::DebugOptions> const*, node::DebugOptions* (node::EnvironmentOptions::*)()) node/src/node_options-inl.h:226:39
    #3 0x5621d1a8b767 in node::options_parser::EnvironmentOptionsParser::EnvironmentOptionsParser() node/src/node_options.cc:301:3
    #4 0x5621d1aa97cc in __cxx_global_var_init.17 node/src/node_options.cc:129:58
    #5 0x5621d1aa97cc in _GLOBAL__sub_I_node_options.cc node/src/node_options.cc
    #6 0x5621d32a1cfc in __libc_csu_init (/usr/local/google/home/yangguo/node-ci/node-ci/out/Release/node_no_cache+0x3689cfc)
    #7 0x7f96be87323f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2023f)
    #8 0x5621d1937029 in _start (/usr/local/google/home/yangguo/node-ci/node-ci/out/Release/node_no_cache+0x1d1f029)

Traceback (most recent call last):
  File "../../tools/generate_code_cache.py", line 15, in <module>
    main(sys.argv[1], sys.argv[2], sys.argv[3])
  File "../../tools/generate_code_cache.py", line 12, in main
    [node_exe, "--expose-internals", script, output])
  File "/usr/lib/python2.7/subprocess.py", line 219, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command '['/usr/local/google/home/yangguo/node-ci/node-ci/out/Release/node_no_cache', '--expose-internals', '../../node/tools/generate_code_cache.js', 'gen/node/node_code_cache.cc']' returned non-zero exit status 1
ninja: build stopped: subcommand failed.
@targos
Copy link
Member

targos commented Feb 15, 2019

@addaleax

@hashseed
Copy link
Member Author

Note that what ubsan detects may not be an actual immediate problem. Rather it points to constructs that are undefined behavior in the C/C++ spec and may change in the future, e.g. when compilers change these behavior to enable better optimizations.

addaleax added a commit to addaleax/node that referenced this issue Feb 15, 2019
@addaleax
Copy link
Member

I couldn’t get this to work locally (without using GN, admittedly), so there might be other issues with UBSAN, but #26139 should address this particular issue.

@hashseed
Copy link
Member Author

Instructions for the GN build:
Get depot_tools. Then

mkdir node-ci
cd node-ci
fetch node-ci
cd node-ci
gclient sync
tools/gn-gen --ubsan-vptr out/Release
ninja -C out/Release node

@addaleax
Copy link
Member

addaleax commented Feb 15, 2019

Okay – It does fix that issues, yes. :)

There’s one other type of issue that pops up repeatedly when running the tests – luckily, there is a 4 year old FIXME for that 😛 :

node/src/req_wrap-inl.h

Lines 20 to 22 in 611043c

// FIXME(bnoordhuis) The fact that a reinterpret_cast is needed is
// arguably a good indicator that there should be more than one queue.
env->req_wrap_queue()->PushBack(reinterpret_cast<ReqWrap<uv_req_t>*>(this));

Should be easy enough to address too, though. Edit: #26148

addaleax added a commit to addaleax/node that referenced this issue Feb 16, 2019
Introduce a second base class for `ReqWrap` that does not
depend on a template parameter and move the `req_wrap_queue_`
field to it.

This addresses undefined behaviour that occurs when casting
to `ReqWrap<uv_req_t>` in the `ReqWrap` constructor.

Refs: nodejs#26131
addaleax added a commit that referenced this issue Feb 18, 2019
Fixes: #26131

PR-URL: #26139
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax added a commit that referenced this issue Feb 20, 2019
Introduce a second base class for `ReqWrap` that does not
depend on a template parameter and move the `req_wrap_queue_`
field to it.

This addresses undefined behaviour that occurs when casting
to `ReqWrap<uv_req_t>` in the `ReqWrap` constructor.

Refs: #26131

PR-URL: #26148
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this issue Feb 21, 2019
Introduce a second base class for `ReqWrap` that does not
depend on a template parameter and move the `req_wrap_queue_`
field to it.

This addresses undefined behaviour that occurs when casting
to `ReqWrap<uv_req_t>` in the `ReqWrap` constructor.

Refs: #26131

PR-URL: #26148
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
Fixes: #26131

PR-URL: #26139
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
Introduce a second base class for `ReqWrap` that does not
depend on a template parameter and move the `req_wrap_queue_`
field to it.

This addresses undefined behaviour that occurs when casting
to `ReqWrap<uv_req_t>` in the `ReqWrap` constructor.

Refs: #26131

PR-URL: #26148
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 a pull request may close this issue.

3 participants