-
Notifications
You must be signed in to change notification settings - Fork 1k
PromiseDisposer: Use NOLINT to disable ArrayBound warning #2417
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
Conversation
Disable clang-analyzer-security.ArrayBound warning in PromiseDisposer::append to silence clang-tidy. A new clang-analyzer-security.ArrayBound "warning: Out of bound access to memory preceding the region" "note: Access of the region at negative byte offset -112" started happening when updating from clang-tidy 20 to 21. The warning doesn't seem to indicate a real problem because ChainPromiseNode instances which inherit from PromiseArenaMember are constructed starting at the end of the PromiseArray byte array and a bounds check is done right before calling the constructor. More information the warning can be found: - https://clang.llvm.org/docs/analyzer/checkers.html#security-arraybound-c-c - bitcoin/bitcoin#33445 (comment) Error output looks like: /usr/include/kj/async-inl.h:404:12: warning: Out of bound access to memory preceding the region [clang-analyzer-security.ArrayBound] 404 | ctor(*ptr, kj::mv(next), kj::fwd<Params>(params)...); | ^ [...] /home/admin/actions-runner/_work/_temp/src/ipc/libmultiprocess/include/mp/type-context.h:146:12: note: Calling 'Promise::then' [...] /usr/include/kj/async-inl.h:1301:7: note: Calling 'maybeChain<capnp::CallContext<ipc::capnp::messages::BlockTemplate::WaitNextParams, ipc::capnp::messages::BlockTemplate::WaitNextResults>>' 1301 | _::maybeChain(kj::mv(intermediate), implicitCast<ResultT*>(nullptr), location)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/kj/async-inl.h:955:10: note: Calling 'appendPromise<kj::_::ChainPromiseNode, kj::SourceLocation &>' 955 | return appendPromise<ChainPromiseNode>(kj::mv(node), location); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/kj/async-inl.h:454:10: note: Calling 'PromiseDisposer::append' 454 | return PromiseDisposer::append<T>(kj::mv(next), kj::fwd<Params>(params)...); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/kj/async-inl.h:389:9: note: Left side of '||' is false 389 | if (!canArenaAllocate<T>() || arena == nullptr || | ^ /usr/include/kj/async-inl.h:389:9: note: Left side of '||' is false /usr/include/kj/async-inl.h:390:9: note: Assuming the condition is false 390 | reinterpret_cast<byte*>(next.get()) - reinterpret_cast<byte*>(arena) < sizeof(T)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/kj/async-inl.h:389:5: note: Taking false branch 389 | if (!canArenaAllocate<T>() || arena == nullptr || | ^ /usr/include/kj/async-inl.h:404:12: note: Access of the region at negative byte offset -112 404 | ctor(*ptr, kj::mv(next), kj::fwd<Params>(params)...); | ^~~~
|
In addition to the warning, it's also possible there may be undefined behavior on the previous line: which ChatGPT suggested changing to something like: to avoid undefined behavior described https://eel.is/c++draft/expr.add#6 where pointer arithmetic is done with a type not matching the underlying array type (which is But I'm not a language lawyer and am not sure how to evaluate this. I also don't know if changing this would affect the ArrayBound warning, so it seemed simplest just to deal with the warning directly. |
|
Sorry for delay. Took me several reads to understand what's going on here, but yeah I guess that makes sense. Thanks! |
|
Thanks! Sorry, comments could have used some editing. To be clear, this PR fixes the ArrayBound warning on line 404 but there may still be undefined behavior on line 403 because the result of the subtraction is undefined. According to https://eel.is/c++draft/expr.add#6, pointer subtraction is defined if the pointer type and underlying array type are compatible, and in the case the pointer type is |
|
Yeah I don't see how anyone can write a memory allocator without violating that rule... |
Disable clang-analyzer-security.ArrayBound warning in PromiseDisposer::append to silence clang-tidy.
More information about the warning can be found in the commit message. A similar change was also made recently in #2334.