Skip to content

Conversation

@ryanofsky
Copy link
Contributor

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.

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)...);
      |            ^~~~
@ryanofsky
Copy link
Contributor Author

In addition to the warning, it's also possible there may be undefined behavior on the previous line:

reinterpret_cast<T*>(next.get()) - 1

which ChatGPT suggested changing to something like:

reinterpret_cast<T*>(reinterpret_cast<unsigned char*>(next.get()) - sizeof(T))

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 byte[1024]).

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.

@kentonv
Copy link
Member

kentonv commented Sep 28, 2025

Sorry for delay. Took me several reads to understand what's going on here, but yeah I guess that makes sense. Thanks!

@kentonv kentonv merged commit d0bab02 into capnproto:master Sep 28, 2025
14 of 16 checks passed
@ryanofsky
Copy link
Contributor Author

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 ChainPromiseNode and array type is byte, so it doesn't seem like that is the case. Realistically, it's hard to see how this could be a problem, but I wanted to point it out in case it wasn't intended.

@kentonv
Copy link
Member

kentonv commented Sep 29, 2025

Yeah I don't see how anyone can write a memory allocator without violating that rule...

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 this pull request may close these issues.

3 participants