deps: update grpc to 1.75.0 #369
Conversation
|
Caution Review failedFailed to post review comments WalkthroughBroad gRPC vendored update: version bumps (Core 50.0.0, C++ 1.75.0), extensive new docs, many build/ignore/config changes, SPIFFE-based TLS root handling via RootCertInfo, Channelz v2→v1 bridge and new ResourceQuota entity, HTTP/2 settings manager and client transport refactors, EventEngine poller moves to ThreadPool, fused filters infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant gRPC_TLS as TLS Cert Provider
participant FS as Filesystem
participant SPIFFE as SPIFFE Bundle Map
participant SecConn as TLS Security Connector
App->>gRPC_TLS: file_watcher_create(priv, cert, root, spiffe_bundle_map_path, interval)
alt spiffe_bundle_map_path provided
gRPC_TLS->>SPIFFE: Load bundle map
SPIFFE-->>gRPC_TLS: RootCertInfo (X509 stack)
else PEM roots
gRPC_TLS->>FS: Read root certs
FS-->>gRPC_TLS: PEM data -> RootCertInfo
end
gRPC_TLS-->>SecConn: OnCertificatesChanged(RootCertInfo, key/cert)
SecConn->>SecConn: Update handshaker factories with RootCertInfo
sequenceDiagram
autonumber
participant Client
participant H2Trans as HTTP/2 Client Transport
participant Settings as Http2SettingsManager
participant Streams as WritableStreams
participant Queue as StreamDataQueue
Client->>H2Trans: Start call/data
H2Trans->>Settings: MaybeSendUpdate()
alt update present
Settings-->>H2Trans: SETTINGS frame
H2Trans->>Peer: Send SETTINGS
end
H2Trans->>Streams: Enqueue stream (priority)
loop Write cycle
Streams-->>H2Trans: Next stream id
H2Trans->>Queue: Dequeue frames (respect FC, header limits)
H2Trans->>Peer: Write frames
end
Peer-->>H2Trans: SETTINGS/ACK, frames
H2Trans->>Settings: ApplyIncomingSettings/ACK
sequenceDiagram
autonumber
participant Tool as Client Tool
participant v1API as Legacy Channelz C API
participant Registry as ChannelzRegistry (v2)
participant Convert as v2→v1 Converters
Tool->>v1API: grpc_channelz_get_channel(id)
v1API->>Registry: Fetch v2 entity/children
Registry-->>v1API: Serialized v2 entity set
v1API->>Convert: ConvertChannel(v2, json=true)
Convert-->>v1API: v1 JSON payload
v1API-->>Tool: JSON string (malloc'ed)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180+ minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (21)
deps/grpc/src/core/ext/filters/stateful_session/GEMINI.md (1)
20-25: Add security/privacy guidance for cookie handling.This filter relies on cookies; the doc should warn about sensitive values, logging, and transport requirements.
## Notes @@ * The filter is designed to be used with xDS, which can be used to dynamically update the stateful session configuration at runtime. * This filter is an important component of gRPC's load balancing story, as it enables applications to maintain session affinity across multiple requests. + +## Security and Privacy +- Treat the affinity cookie value as sensitive. Avoid logging it and ensure redaction in client logs/metrics. +- Use TLS; do not rely on cookies over plaintext transports. +- Prefer opaque/non-PII tokens; avoid embedding user identifiers directly. +- Consider rotation/expiry semantics so stale affinity does not pin clients indefinitely. + +## Configuration +Add an example service config/xDS snippet showing how to: +- specify the cookie name and any parsing/validation parameters, +- enable the filter alongside the `xds_override_host` policy, +- define fallback behavior when the cookie is missing. +(Please populate from `StatefulSessionServiceConfigParser` to avoid drift.)I can auto‑extract the config keys from the parser and draft this section. Want me to generate it?
deps/grpc/src/core/ext/filters/census/GEMINI.md (1)
1-23: Action required: Census/OpenCensus filter is still compiled/enabled in default builds — remove or gate itRipgrep shows census sources are referenced by default build files, public headers, and runtime flags (defaults enabled). Remove these from default targets or gate them behind a build-time feature flag and set runtime defaults to disabled.
- Build files: deps/grpc/grpc.gyp and deps/grpc/Makefile include src/core/ext/filters/census/grpc_context.cc.
- Bazel/third-party: deps/grpc/src/cpp/ext/gcp/BUILD and deps/grpc/third_party/BUILD expose //:grpc_opencensus_plugin and opencensus aliases.
- Public API/headers: deps/grpc/include/grpc/grpc.h and deps/grpc/include/grpcpp/{client_context.h,server_context.h} expose census_context and propagation options.
- Runtime flag: deps/grpc/src/cpp/ext/filters/census/grpc_plugin.cc defines g_open_census_stats_enabled and g_open_census_tracing_enabled (default true).
- Coupling: deps/grpc/src/cpp/ext/otel/* reuse census_context (OTel integration depends on the same type).
Fix: remove/conditionalize census source files from Makefile/gyp/BUILD default targets, ensure public headers do not expose census APIs when implementation is excluded, and set g_open_census_* defaults to false (or provide a documented opt-in).
deps/grpc/src/core/ext/transport/inproc/GEMINI.md (1)
27-27: Correct claim about serialization/deserialization.The C/C++ surfaces still marshal requests into ByteBuffer/serialized form, and servers unmarshal them; inproc primarily avoids network framing/syscalls. Claiming “no need to serialize and deserialize” is misleading.
Apply:
-* When using the in-process transport, the client and server are running in the same process, so they can share the same memory. This means that there is no need to serialize and deserialize messages, which can lead to significant performance improvements. +* When using the in‑process transport, the client and server run in the same process, avoiding network framing and syscalls. Application‑level message serialization/deserialization still occurs, but data may be handed off with fewer copies, yielding lower latency and CPU overhead than remote transports.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc (1)
426-429: Bug: zeroing header padding uses the wrong length.memset uses frame_padding instead of header_padding, which can underrun/overrun the intended region for the header’s padding area.
Apply this diff:
- if (header_padding != 0) { - memset(slice.data() + TcpDataFrameHeader::kFrameHeaderSize, 0, - frame_padding); - } + if (header_padding != 0) { + memset(slice.data() + TcpDataFrameHeader::kFrameHeaderSize, 0, + header_padding); + }deps/grpc/src/core/lib/iomgr/exec_ctx.h (1)
24-36: Add missing standard headers used by this file.std::optional and abort() are used but the corresponding headers aren’t explicitly included in this header. Relying on transitive includes is brittle.
#include <limits> +#include <optional> // for std::optional +#include <cstdlib> // for abortdeps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h (1)
125-138: Add 32-bit overflow guard for CONTINUATION frames (symmetry with HEADERS path).On 32‑bit builds,
AppendContinuationFrame()can overflowSliceBufferlength;AppendHeaderFrame()already guards this.Apply this diff:
Http2Status AppendContinuationFrame(Http2ContinuationFrame&& frame) { - // Manage payload - const size_t current_len = frame.payload.Length(); + // Manage payload + const size_t current_len = frame.payload.Length(); + if constexpr (sizeof(size_t) == 4) { + if (GPR_UNLIKELY(buffer_.Length() >= UINT32_MAX - current_len)) { + Cleanup(); + LOG(ERROR) << "Stream Error: SliceBuffer overflow for 32 bit platforms."; + return Http2Status::Http2StreamError( + Http2ErrorCode::kInternalError, + "Stream Error: SliceBuffer overflow for 32 bit platforms."); + } + } frame.payload.MoveFirstNBytesIntoSliceBuffer(current_len, buffer_); ASSEMBLER_LOG << "AppendContinuationFrame " << current_len << " Bytes.";deps/grpc/src/core/lib/iomgr/tcp_posix.cc (1)
1324-1325: Build‑breaking cast: missing parentheses in reinterpret_cast.This line won’t compile; the cast must wrap CMSG_DATA(...).
Apply this fix:
- auto serr = reinterpret_cast<const sock_extended_err*> CMSG_DATA(&cmsg); + auto serr = reinterpret_cast<const sock_extended_err*>(CMSG_DATA(&cmsg));deps/grpc/src/core/lib/surface/call.cc (1)
498-503: Verify the return type inconsistency.The function signature indicates
voidreturn type, but Line 502 has areturnstatement with a value. This appears to be a typo.Apply this diff to fix the return type inconsistency:
- return arena->SetContext<grpc_core::CallTracerAnnotationInterface>(tracer); + arena->SetContext<grpc_core::CallTracerAnnotationInterface>(tracer);deps/grpc/src/core/call/filter_fusion.h (1)
1132-1221: Document the kFlags template parameter and its semantics.
- Add docs listing the available flags (defined in deps/grpc/src/core/lib/channel/promise_based_filter.h — e.g. kFilterExaminesServerInitialMetadata) and precisely describe their effects (e.g. when kFilterExaminesServerInitialMetadata is set the filter receives/creates the server_initial_metadata latch and may only examine vs mutate metadata).
- Update FusedFilter docs (deps/grpc/src/core/call/filter_fusion.h) and public CHANGELOG/upgrade notes with a migration guide: default = 0 (no flags), how to convert existing MakePromiseBasedFilter/FusedFilter usages to the new kFlags parameter, and an example usage (see src/core/ext/filters/http/client/http_client_filter.cc).
deps/grpc/src/core/lib/event_engine/cf_engine/cf_engine.h (1)
33-61: Missing forward declaration forThreadPoolcauses header compile error.
ThreadPoolis used in the header but not declared or included. Add a forward decl in the correct namespace (preferred) or include the header.namespace grpc_event_engine::experimental { +class ThreadPool; // forward declaration + class CFEventEngine : public EventEngine {Alternatively, include:
-#include "src/core/lib/event_engine/posix_engine/timer_manager.h" +#include "src/core/lib/event_engine/posix_engine/timer_manager.h" +#include "src/core/lib/event_engine/thread_pool/thread_pool.h"deps/grpc/src/core/BUILD (2)
3336-3349: Fix visibility: //bazel:socket_mutator package_group missingdeps/grpc/src/core/BUILD (around lines 3334–3346) sets visibility = ["//bazel:socket_mutator"] but no package_group named "socket_mutator" was found in the repo — Bazel will fail visibility checks.
- Add package_group(name = "socket_mutator") under bazel/BUILD with the appropriate packages, or change visibility to an existing package_group or //visibility:public.
8038-8086: HTTP/2 settings manager wiring — add missing BUILD deps
- Add //:http2_settings_manager to the deps of targets that include/use it: chttp2_flow_control (deps/grpc/src/core/ext/transport/chttp2/transport/flow_control.h, .cc), http2_client_transport (…/http2_client_transport.h, .cc), http2_server_transport (…/http2_server_transport.h).
- Also verify/ensure the BUILD owners of deps/grpc/src/core/ext/transport/chttp2/transport/parsing.cc, chttp2_transport.cc, and internal.h list //:http2_settings_manager as a dep.
deps/grpc/src/core/lib/event_engine/posix_engine/posix_endpoint.cc (3)
126-150: Fix parsing bug in ParseUlimitMemLockFromFile (wrong std::string::find overload and substr length).
- Currently calls find(start, '\n') which swaps args; it resolves to find(char, pos), using start as a char and '\n' as pos. Also substr length is off.
Apply this diff:
- // Find position of next newline after prefix. - size_t end = file_contents.find(start, '\n'); - // Extract substring between prefix and next newline. - auto memlock_value_string = file_contents.substr( - start + kHardMemlockPrefix.length() + 1, end - start); + // Find position of next newline after prefix. + size_t end = file_contents.find('\n', start); + if (end == std::string::npos) end = file_contents.size(); + // Extract substring between prefix and next newline (skip whitespace). + size_t value_start = + file_contents.find_first_not_of(" \t", start + kHardMemlockPrefix.length()); + if (value_start == std::string::npos || value_start >= end) { + return 0; + } + auto memlock_value_string = file_contents.substr(value_start, end - value_start);
166-177: Close DIR before early return to avoid descriptor leak.If a valid memlock is found, we return without closedir(dir).
- if (hard_memlock != 0) { - return hard_memlock; - } + if (hard_memlock != 0) { + closedir(dir); + return hard_memlock; + }
1315-1333: Inverted logic disables zero‑copy on success; should only disable/log on failure.Currently, SetSockOpt(...).ok() triggers error path. This disables zerocopy even when SO_ZEROCOPY was set successfully.
- if (posix_interface.SetSockOpt(fd, SOL_SOCKET, SO_ZEROCOPY, 1).ok()) { - zerocopy_enabled = false; - LOG(ERROR) << "Failed to set zerocopy options on the socket."; - } + auto rc = posix_interface.SetSockOpt(fd, SOL_SOCKET, SO_ZEROCOPY, 1); + if (!rc.ok()) { + zerocopy_enabled = false; + LOG(ERROR) << "Failed to set zerocopy options on the socket: " << rc.StrError(); + }deps/grpc/src/core/client_channel/load_balanced_call_destination.cc (1)
31-36: Context type mismatch breaks attempt tracer propagation.You set a ClientCallTracerInterface::CallAttemptTracer* into the context under CallTracerInterface, but later retrieve using ClientCallTracerInterface::CallAttemptTracer. These type keys won’t match; tracer will be lost.
Apply:
- auto* tracer = call_tracer->StartNewAttempt(is_transparent_retry); - SetContext<CallTracerInterface>(tracer); + auto* tracer = call_tracer->StartNewAttempt(is_transparent_retry); + SetContext<ClientCallTracerInterface::CallAttemptTracer>(tracer);deps/grpc/src/core/channelz/property_list.cc (1)
48-51: Fix leak: gpr_format_timespec() returns heap memory that must be freed.The result is converted to string but never freed. Free after copying.
- [](Timestamp v) { - return Json::FromString( - gpr_format_timespec(v.as_timespec(GPR_CLOCK_REALTIME))); - }, + [](Timestamp v) { + char* buf = gpr_format_timespec(v.as_timespec(GPR_CLOCK_REALTIME)); + std::string s(buf ? buf : ""); + gpr_free(buf); + return Json::FromString(std::move(s)); + },deps/grpc/src/core/credentials/transport/tls/spiffe_utils.cc (1)
25-34: Missing includes for CHECK and ascii helpers.CHECK is used; absl::ascii_isascii is used. Add required headers.
#include <string> +#include "absl/log/check.h" +#include "absl/strings/ascii.h" #include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h"deps/grpc/src/core/lib/resource_quota/memory_quota.h (1)
286-303: Ensure DataSource is properly registered/unregisteredCtor must initialize DataSource with channelz_node and call SourceConstructed(); dtor already calls SourceDestructing().
If missing, update the definition:
-BasicMemoryQuota::BasicMemoryQuota(RefCountedPtr<channelz::ResourceQuotaNode> channelz_node) - : /* ... */ { - /* ... */ -} +BasicMemoryQuota::BasicMemoryQuota(RefCountedPtr<channelz::ResourceQuotaNode> channelz_node) + : channelz::DataSource(channelz_node) /*, ... */ { + SourceConstructed(); + /* ... */ +}deps/grpc/src/core/lib/event_engine/posix_engine/posix_engine.cc (2)
213-221: Data race on done_ in PollingCycle dtordone_ is written without holding mu_, but read under mu_ in PollerWorkInternal(). That’s a data race under the C++ memory model.
Fix by writing done_ under mu_:
-PosixEventEngine::PollingCycle::~PollingCycle() { - done_ = true; - poller_->Kick(); - grpc_core::MutexLock lock(&mu_); +PosixEventEngine::PollingCycle::~PollingCycle() { + grpc_core::MutexLock lock(&mu_); + done_ = true; + poller_->Kick(); while (is_scheduled_ > 0) { cond_.Wait(&mu_); } }Follow‑up: declare done_ as ABSL_GUARDED_BY(mu_) (or make it std::atomic) in the class definition.
490-497: Test engine doesn’t start polling; schedule the pollerMakeTestOnlyPosixEventEngine constructs an engine with a poller but never schedules it, unlike the default ctor. Tests relying on real polling will stall.
std::shared_ptr<PosixEventEngine> engine( new PosixEventEngine(std::move(test_only_poller))); RegisterEventEngineForFork(engine, engine->executor_, engine->timer_manager_); +engine->SchedulePoller(); return engine;
| "lib/promise/poll.h", | ||
| ], | ||
| visibility = ["//bazel:core_credentials"], | ||
| deps = [ |
There was a problem hiding this comment.
🧩 Analysis chain
Visibility tightening (poll, time, context, seq, map, arena_promise).
Tightening to //bazel:core_credentials is fine intra-package, but check for external package usages that will break.
Also applies to: 1985-1986, 817-817, 832-838, 897-905, 1153-1156
🏁 Script executed:
#!/bin/bash
# Find deps on the restricted libs outside //src/core
for lib in poll time context seq map arena_promise; do
rg -nP "(deps\\s*=\\s*\\[|select_deps).*\"//src/core:${lib}\"" -g '!deps/grpc/src/core/**' -C2
doneLength of output: 893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
libs_pattern='(poll|time|context|seq|map|arena_promise)'
echo "Searching repository for references to //src/core:${libs_pattern} (including ignored/hidden files)..."
matches=$(rg -uu -n --hidden --color=never -S "//src/core:${libs_pattern}" -C2 || true)
if [ -z "$matches" ]; then
echo "No matches found."
exit 0
fi
echo "=== ALL MATCHES ==="
printf "%s\n" "$matches"
echo
echo "=== EXTERNAL MATCHES (excluding deps/grpc/src/core/) ==="
printf "%s\n" "$matches" | rg -v "deps/grpc/src/core/" || echo "No external matches found."Length of output: 2832
Visibility tightening will break external consumers — do not change visibility yet.
External BUILD files reference the //src/core targets:
- deps/grpc/src/cpp/ext/otel/BUILD:75 — "//src/core:arena_promise"
- deps/grpc/src/cpp/ext/otel/BUILD:79 — "//src/core:context"
- deps/grpc/src/cpp/ext/gcp/BUILD:144 — "//src/core:time"
- deps/grpc/src/cpp/ext/gcp/BUILD:190 — "//src/core:time"
Do not tighten visibility to //bazel:core_credentials until these consumers are updated or an alias/re-export is provided.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around line 721, the attempted visibility tightening
to //bazel:core_credentials will break external consumers that reference
//src/core targets (see listed BUILD files); do not change the visibility.
Instead, revert the visibility change or restore public visibility for the
affected targets, and if the intent is to restrict access, add an alias or
re-export target (e.g., //bazel:core_credentials) that wraps the original target
and update dependent internal targets to use the alias; only tighten visibility
after updating all external BUILD files or providing the alias/re-export so
external consumers are not broken.
| grpc_cc_library( | ||
| name = "fused_filters", | ||
| srcs = [ | ||
| "filter/fused_filters.cc", | ||
| ], | ||
| defines = select({ | ||
| ":grpc_no_filter_fusion": ["GRPC_NO_FILTER_FUSION"], | ||
| "//conditions:default": [], | ||
| }), | ||
| select_deps = [{ | ||
| ":grpc_no_filter_fusion": [], | ||
| "//conditions:default": [ | ||
| "experiments", | ||
| "filter_fusion", | ||
| "grpc_authorization_base", | ||
| "grpc_client_authority_filter", | ||
| "grpc_lb_policy_grpclb", | ||
| "grpc_message_size_filter", | ||
| "server_call_tracer_filter", | ||
| "service_config_channel_arg_filter", | ||
| "//:grpc_http_filters", | ||
| "//:grpc_security_base", | ||
| ], | ||
| }], | ||
| deps = [ | ||
| "//:config", | ||
| "//:gpr", | ||
| "//:grpc_public_hdrs", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
New fused_filters target: link/gating check.
Looks sane; gated correctly with select_deps. Please confirm all deps are present in this BUILD (esp. grpc_authorization_base, grpc_lb_policy_grpclb, service_config_channel_arg_filter, and //:grpc_http_filters) and that fusion stays off when :grpc_no_filter_fusion matches.
🏁 Script executed:
#!/bin/bash
# Verify all fused_filters deps exist
for t in experiments filter_fusion grpc_authorization_base grpc_client_authority_filter grpc_lb_policy_grpclb grpc_message_size_filter server_call_tracer_filter service_config_channel_arg_filter //:grpc_http_filters //:grpc_security_base; do
rg -nP "(name\\s*=\\s*\"${t#//:}\")|(: ${t})" deps/grpc/src/core/BUILD -C1 || echo "MISSING: $t"
doneLength of output: 712
fused_filters: missing //:grpc_http_filters and //:grpc_security_base — fix required
- deps/grpc/src/core/BUILD: fused_filters references //:grpc_http_filters and //:grpc_security_base but those targets are not defined in this BUILD (script output). Add the targets or correct the labels.
- Gating: select_deps maps :grpc_no_filter_fusion -> [] so fusion is disabled when that config matches; confirm :grpc_no_filter_fusion is defined as a config_setting and wired into your build config.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 3666 to 3695, the fused_filters target
references //:grpc_http_filters and //:grpc_security_base which are not defined
in this BUILD; add those targets or correct the labels to the proper package
paths (or replace them with existing targets), and ensure the select_deps
mapping points to valid labels; additionally verify that :grpc_no_filter_fusion
is declared as a config_setting elsewhere and wired into the build config (or
add that config_setting here) so the select() branches are resolvable.
| name = "auth_context_comparator_registry", | ||
| hdrs = [ | ||
| "transport/auth_context_comparator_registry.h", | ||
| ], | ||
| external_deps = [ | ||
| "absl/functional:any_invocable", | ||
| "absl/strings", | ||
| ], | ||
| visibility = ["//transport_auth_context:__subpackages__"], | ||
| deps = [ | ||
| "channel_args", | ||
| "//:grpc_public_hdrs", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
New auth_context_comparator_registry: verify visibility package.
visibility = ["//transport_auth_context:__subpackages__"] assumes a top-level transport_auth_context package. Confirm it exists; otherwise switch to a package group or public visibility as intended.
🏁 Script executed:
#!/bin/bash
fd -t f -a transport_auth_context | wc -l
rg -nP --type=bzl 'package\(|package_group\(|name\s*=\s*"transport_auth_context"' -C2Length of output: 29
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find directories named transport_auth_context"
fd -HI -t d transport_auth_context || true
echo
echo "2) Files containing the string 'transport_auth_context'"
rg -n --hidden --no-ignore-vcs -S 'transport_auth_context' || true
echo
echo "3) Exact visibility label occurrences"
rg -n --hidden --no-ignore-vcs -S '"//transport_auth_context:__subpackages__"' || true
echo
echo "4) package_group or package declarations mentioning transport_auth_context"
rg -n --hidden --no-ignore-vcs -S 'package_group\(|package\(|name\s*=\s*"(transport_auth_context)"' -C2 || trueLength of output: 262181
Invalid visibility: //transport_auth_context:__subpackages__ does not exist
Update the visibility to an existing package, for example:
//deps/grpc/src/core/transport:__subpackages__to expose only thetransportsubpackages- or
//visibility:publicif the header should be globally visible
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 10075 to 10088 the target's visibility
entry uses a non-existent package //transport_auth_context:__subpackages__,
causing an invalid visibility error; replace that visibility with a valid
package path such as //deps/grpc/src/core/transport:__subpackages__ to expose
only transport subpackages, or use //visibility:public if the header must be
globally visible, and update the BUILD file accordingly.
| absl::Status ServerMetadataToStatus(ServerMetadata& md) { | ||
| const auto grpc_status = | ||
| md.get(GrpcStatusMetadata()).value_or(GRPC_STATUS_UNKNOWN); | ||
| const auto* error_slice = md.get_pointer(GrpcMessageMetadata()); | ||
| return grpc_error_set_int( | ||
| absl::Status(static_cast<absl::StatusCode>(grpc_status), | ||
| error_slice->as_string_view()), | ||
| StatusIntProperty::kRpcStatus, grpc_status); | ||
| } |
There was a problem hiding this comment.
Potential null pointer dereference in ServerMetadataToStatus.
The function dereferences error_slice on line 43 without checking if get_pointer returned null. This could cause a crash if GrpcMessageMetadata is not present in the metadata.
Apply this fix to handle the null case safely:
absl::Status ServerMetadataToStatus(ServerMetadata& md) {
const auto grpc_status =
md.get(GrpcStatusMetadata()).value_or(GRPC_STATUS_UNKNOWN);
const auto* error_slice = md.get_pointer(GrpcMessageMetadata());
+ std::string_view message_view = error_slice ? error_slice->as_string_view() : "";
return grpc_error_set_int(
absl::Status(static_cast<absl::StatusCode>(grpc_status),
- error_slice->as_string_view()),
+ message_view),
StatusIntProperty::kRpcStatus, grpc_status);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| absl::Status ServerMetadataToStatus(ServerMetadata& md) { | |
| const auto grpc_status = | |
| md.get(GrpcStatusMetadata()).value_or(GRPC_STATUS_UNKNOWN); | |
| const auto* error_slice = md.get_pointer(GrpcMessageMetadata()); | |
| return grpc_error_set_int( | |
| absl::Status(static_cast<absl::StatusCode>(grpc_status), | |
| error_slice->as_string_view()), | |
| StatusIntProperty::kRpcStatus, grpc_status); | |
| } | |
| absl::Status ServerMetadataToStatus(ServerMetadata& md) { | |
| const auto grpc_status = | |
| md.get(GrpcStatusMetadata()).value_or(GRPC_STATUS_UNKNOWN); | |
| const auto* error_slice = md.get_pointer(GrpcMessageMetadata()); | |
| std::string_view message_view = error_slice ? error_slice->as_string_view() : ""; | |
| return grpc_error_set_int( | |
| absl::Status(static_cast<absl::StatusCode>(grpc_status), | |
| message_view), | |
| StatusIntProperty::kRpcStatus, grpc_status); | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/call/metadata.cc around lines 37 to 45,
ServerMetadataToStatus dereferences error_slice returned from
md.get_pointer(GrpcMessageMetadata()) without a null check; update the function
to safely handle the null case by checking if error_slice is non-null before
using it and using an empty string (or
md.get(GrpcMessageMetadata()).value_or("") result) when it is null, then
construct the absl::Status with that safe string and return the
grpc_error_set_int call as before.
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "absl/status/statusor.h" | ||
|
|
There was a problem hiding this comment.
Missing include for int64_t.
Public header uses int64_t but doesn’t include <cstdint>. This will break some compilers/translation units.
Apply this diff:
#include <string>
#include <vector>
+#include <cstdint>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <string> | |
| #include <vector> | |
| #include "absl/status/statusor.h" | |
| #include <string> | |
| #include <vector> | |
| #include <cstdint> | |
| #include "absl/status/statusor.h" |
🤖 Prompt for AI Agents
In deps/grpc/src/core/channelz/v2tov1/convert.h around lines 18 to 22 the public
header uses int64_t but does not include <cstdint>, which can break some
compilers; add a #include <cstdint> to the header alongside the existing
includes so int64_t is declared for all translation units that include this
header.
| #include "src/core/lib/event_engine/posix_engine/tcp_socket_utils.h" | ||
| #include "src/core/lib/event_engine/tcp_socket_utils.h" | ||
| #include "src/core/lib/iomgr/socket_mutator.h" | ||
| #include "src/core/net/socket_mutator.h" |
There was a problem hiding this comment.
🧩 Analysis chain
Header moved to src/core/net: ensure matching .cc is linked (avoid ODR/link errors).
Include path switched to src/core/net/socket_mutator.h. Verify the build links the corresponding implementation (likely src/core/net/socket_mutator.cc) and not the old src/core/lib/iomgr/socket_mutator.cc. Mismatch will break or cause duplicate symbol issues.
🏁 Script executed:
#!/bin/bash
# Inspect header/impl paths and gyp wiring for socket_mutator.
set -euo pipefail
echo "Files present:"
fd -a --strip-cwd-prefix 'socket_mutator\.(cc|h)' deps/grpc/src | sort
echo
echo "Who includes socket_mutator.h:"
rg -n 'socket_mutator\.h' deps/grpc/src -C2
echo
echo "Which impl path is referenced in grpc.gyp:"
rg -nP 'src/core/(lib/iomgr|net)/socket_mutator\.cc' deps/grpc/grpc.gypLength of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) List socket_mutator.(cc|h) under deps/grpc/src ==="
if command -v fd >/dev/null 2>&1; then
fd -a 'socket_mutator\.(cc|h)' deps/grpc/src || true
else
find deps/grpc/src -type f \( -name 'socket_mutator.cc' -o -name 'socket_mutator.h' \) -print || true
fi
echo
echo "=== 2) Which files include socket_mutator.h under deps/grpc ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -S 'socket_mutator\.h' deps/grpc || true
else
grep -RIn --line-number --binary-files=without-match 'socket_mutator\.h' deps/grpc || true
fi
echo
echo "=== 3) Where socket_mutator.cc is referenced in build files under deps/grpc ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -S 'socket_mutator\.cc' deps/grpc || true
else
grep -RIn --line-number --binary-files=without-match 'socket_mutator\.cc' deps/grpc || true
fi
echo
echo "=== 4) Show include lines that mention socket_mutator.h ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -S '#include.*socket_mutator\.h' deps/grpc || true
else
grep -RIn --line-number --binary-files=without-match '#include.*socket_mutator\.h' deps/grpc || true
fiLength of output: 2751
Link only one socket_mutator implementation — fix build files
Both src/core/lib/iomgr/socket_mutator.cc and src/core/net/socket_mutator.cc exist and are referenced by the build (net header currently includes the lib header/TODO). Remove or consolidate so only one .cc defines/gets linked to avoid duplicate-symbol/ODR errors.
Update the following (remove the unwanted .cc or make it a non-duplicating wrapper):
- deps/grpc/src/core/lib/iomgr/socket_mutator.cc
- deps/grpc/src/core/net/socket_mutator.cc
- Build entries: deps/grpc/grpc.gyp, deps/grpc/Makefile, deps/grpc/src/core/BUILD (they currently list both implementations)
🤖 Prompt for AI Agents
In deps/grpc/src/core/lib/event_engine/posix_engine/posix_engine_listener.cc
around line 47, the project is compiling two conflicting socket_mutator
implementations (src/core/lib/iomgr/socket_mutator.cc and
src/core/net/socket_mutator.cc) causing duplicate-symbol/ODR errors; fix this by
keeping a single implementation and updating build entries: delete or convert
one of the .cc files (either remove
deps/grpc/src/core/lib/iomgr/socket_mutator.cc or
deps/grpc/src/core/net/socket_mutator.cc, or make the removed one a thin
non-defining wrapper/header-only adaptor), and then update deps/grpc/grpc.gyp,
deps/grpc/Makefile, and deps/grpc/src/core/BUILD to reference only the retained
implementation so only one .cc is compiled/linked.
| #if defined(GRPC_POSIX_SOCKET_TCP) && \ | ||
| !defined(GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER) | ||
| #define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING true | ||
| #else | ||
| #define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING false | ||
| #endif |
There was a problem hiding this comment.
Preprocessor constant uses true/false; breaks #if evaluation.
In #if expressions, define to 1/0, not true/false, otherwise GRPC_PLATFORM_SUPPORTS_POSIX_POLLING may evaluate as 0.
-#if defined(GRPC_POSIX_SOCKET_TCP) && \
- !defined(GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER)
-#define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING true
-#else
-#define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING false
-#endif
+#if defined(GRPC_POSIX_SOCKET_TCP) && \
+ !defined(GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER)
+#define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING 1
+#else
+#define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING 0
+#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if defined(GRPC_POSIX_SOCKET_TCP) && \ | |
| !defined(GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER) | |
| #define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING true | |
| #else | |
| #define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING false | |
| #endif | |
| #if defined(GRPC_POSIX_SOCKET_TCP) && \ | |
| !defined(GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER) | |
| #define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING 1 | |
| #else | |
| #define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING 0 | |
| #endif |
🤖 Prompt for AI Agents
In deps/grpc/src/core/lib/event_engine/posix_engine/posix_engine.h around lines
51-56 the macro GRPC_PLATFORM_SUPPORTS_POSIX_POLLING is being defined to the
tokens true/false which break #if evaluation; change the definitions to numeric
1 and 0 (i.e., #define GRPC_PLATFORM_SUPPORTS_POSIX_POLLING 1 and #define
GRPC_PLATFORM_SUPPORTS_POSIX_POLLING 0) so preprocessor #if checks evaluate
correctly.
| #include "src/core/lib/iomgr/socket_utils_posix.h" | ||
| #include "src/core/lib/resource_quota/resource_quota.h" | ||
| #include "src/core/net/socket_mutator.h" | ||
| #include "src/core/util/useful.h" |
There was a problem hiding this comment.
🧩 Analysis chain
Header relocation: check for stale includes and build rules.
Including src/core/net/socket_mutator.h looks correct; verify no remaining includes reference the old src/core/lib/iomgr/socket_mutator.h and that build files export the new header.
Use:
🏁 Script executed:
#!/bin/bash
rg -n 'src/core/lib/iomgr/socket_mutator.h' deps/grpc || true
fd -a socket_mutator.h deps/grpc/src | sortLength of output: 317
Remove stale include and update build exports
- deps/grpc/src/core/net/socket_mutator.h (lines 22–23) still includes "src/core/lib/iomgr/socket_mutator.h" — replace with "src/core/net/socket_mutator.h" or remove the stale reference.
- Ensure build files export deps/grpc/src/core/net/socket_mutator.h and remove any exports/references to src/core/lib/iomgr/socket_mutator.h.
🤖 Prompt for AI Agents
In deps/grpc/src/core/lib/iomgr/socket_utils_posix.cc around line 46, there is a
stale header/reference issue arising from
deps/grpc/src/core/net/socket_mutator.h still including
"src/core/lib/iomgr/socket_mutator.h" (lines 22–23); update that include to
"src/core/net/socket_mutator.h" or remove it if redundant, and then update the
build files to export deps/grpc/src/core/net/socket_mutator.h while removing any
exports or references to src/core/lib/iomgr/socket_mutator.h so the build uses
the correct header path.
| #include "src/core/lib/iomgr/resolved_address.h" | ||
| #include "src/core/lib/iomgr/socket_mutator.h" | ||
| #include "src/core/lib/resource_quota/resource_quota.h" | ||
| #include "src/core/net/socket_mutator.h" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Include path update acknowledged.
API surface unchanged; ensure downstream includes don’t reference the old iomgr header.
🏁 Script executed:
#!/bin/bash
rg -n 'src/core/lib/iomgr/socket_mutator\.h'Length of output: 193
Include path update acknowledged — remaining downstream include found
deps/grpc/src/core/net/socket_mutator.h still #includes "src/core/lib/iomgr/socket_mutator.h" (lines 22–23). Update that include to the new header (or add a forwarding header) so no downstream file references the old iomgr path.
🤖 Prompt for AI Agents
In deps/grpc/src/core/net/socket_mutator.h around lines 22–23, the file still
includes the old path "src/core/lib/iomgr/socket_mutator.h"; update that include
to reference the new header (for example
"src/core/lib/iomgr/socket_utils_posix.h") or create a forwarding header at the
old location that simply includes the new header. Change the #include line
accordingly or add the forwarding header with proper include guards so
downstream files no longer reference the old iomgr path.
| #include "src/core/lib/iomgr/unix_sockets_posix.h" | ||
| #include "src/core/lib/iomgr/vsock.h" | ||
| #include "src/core/lib/slice/slice_internal.h" | ||
| #include "src/core/net/socket_mutator.h" |
There was a problem hiding this comment.
🧩 Analysis chain
Header move: confirm include path/bazel/cmake wiring aligns with gRPC 1.75.0.
The include switched to src/core/net/socket_mutator.h. Looks right for 1.75.x, but please ensure our vendored build files (GN/Bazel/CMake) expose this new path and that no files still include the old iomgr header.
Run to verify no stale includes remain and the new header exists:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) No old includes
rg -n '^\s*#\s*include\s+"src/core/lib/iomgr/socket_mutator\.h"' -S || echo "OK: no old includes"
# 2) New header present
fd socket_mutator.h deps/grpc/src | rg -n 'src/core/net/socket_mutator\.h' -n || { echo "Missing src/core/net/socket_mutator.h"; exit 1; }Length of output: 131
Header move: fix stale include in new header (deps/grpc/src/core/net/socket_mutator.h:23).
deps/grpc/src/core/net/socket_mutator.h line 23 still contains #include "src/core/lib/iomgr/socket_mutator.h" — remove or update that include to the new public header and update vendored BUILD/Bazel/CMake/GN wiring so consumers use src/core/net/socket_mutator.h; re-scan repo for remaining stale includes.
🤖 Prompt for AI Agents
In deps/grpc/src/core/lib/iomgr/tcp_client_posix.cc around line 54, there is a
stale internal header reference originating from
deps/grpc/src/core/net/socket_mutator.h line 23 which still includes
"src/core/lib/iomgr/socket_mutator.h"; edit
deps/grpc/src/core/net/socket_mutator.h to remove or replace that old include
with the new public header (or a forward declaration) so the net header is
self-contained, then update the vendored BUILD/Bazel/CMake/GN files to export
and consume src/core/net/socket_mutator.h instead of the old iomgr path, and
finally re-scan the repo for any remaining occurrences of the old include and
fix them similarly.
santigimeno
changed the base branch from
node-v22.x-nsolid-v5.x
to
node-v22.x-nsolid-v6.x
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores