CI Robot AI Test - Fixing some Coverity-reported Issues#3
Open
CI Robot AI Test - Fixing some Coverity-reported Issues#3
Conversation
Coverity reported an issue where the oftable->vacancy_up/down members were read without holding the ofproto_mutex. This patch ensures the mutex is acquired before accessing these members and adds OVS_REQUIRES(ofproto_mutex) to the affected function to enforce proper locking. Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Fixes: bab8601 ("Implement Vacancy Events for OFPMP_TABLE_DESC.") Signed-off-by: 0-day Robot <robot@bytheb.org>
This patch resolves a Coverity-reported issue where a dereference could occur after a null check in ovsdb_monitor_row_update_type(). An additional condition has been added to ensure a safe early exit in ovsdb_monitor_compose_row_update(). Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Coverity reported a possible null dereference of t->reply when handling errors in the "committing" state for "transact" requests. In practice, t->reply is always non-NULL at this point, but static analysis cannot infer this guarantee. Adding ovs_assert(t->reply) documents the invariant for developers and ensures debug builds will catch any misuse. No change in runtime behavior; this only improves code safety and satisfies static analysis. Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Coverity reported a possible null dereference of ctx->db inside the event processing loop in ovsdb_relay_run(). In practice, ctx->db is guaranteed to be non-NULL for valid relay_ctx structures. Adding ovs_assert(ctx->db) documents this invariant and ensures debug builds catch any misuse. No change in runtime behavior; this only improves code safety and satisfies static analysis. Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Coverity flagged a potential null dereference of txnid when calling raft_next_entry() in ovsdb_storage_read(). To prevent this, raft_next_entry() now checks whether txnid is non-NULL before attempting to write to it. This preserves existing behavior when txnid is valid, while safely handling the case where it is NULL. No change to normal operation; this only prevents potential crashes if a NULL txnid is passed. Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Coverity reported a possible null dereference of datum->values when handling value-type weak references in find_and_add_weak_refs(). In practice, datum->values must be non-NULL for value-type weak references. Adding ovs_assert(datum->values) documents this invariant and ensures that we catch any unexpected misuse. No change to runtime behavior in production builds; this only improves safety and satisfies static analysis. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> v2: Move the assert out of the loop + !datum->n check. Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduces thread-safety annotations and mutex locks for ofproto table operations, adds runtime assertions and null-pointer checks across raft, monitor, relay, transaction, and trigger modules to address Coverity-reported issues. Class diagram for updated ofproto table operations (thread-safety annotations and locking)classDiagram
class ofproto {
+tables[]
+n_tables
}
class oftable {
}
class ofputil_table_desc {
}
class query_table_desc__ {
+OVS_REQUIRES(ofproto_mutex)
}
class query_tables_desc {
+ovs_mutex_lock(&ofproto_mutex)
+ovs_mutex_unlock(&ofproto_mutex)
}
ofproto "1" -- "*" oftable : contains
ofproto "1" -- "*" ofputil_table_desc : describes
query_tables_desc ..> query_table_desc__ : calls
query_table_desc__ ..> ofproto : accesses
query_table_desc__ ..> oftable : accesses
query_tables_desc ..> ofproto : accesses
query_tables_desc ..> ofputil_table_desc : allocates
Class diagram for updated ovsdb_monitor_row_update_type (conditional logic fix)classDiagram
class ovsdb_monitor_row_update_type {
+initial
+old
+new
+return initial ? (new ? OJMS_INITIAL : OJMS_NONE) : !old ? OJMS_INSERT : !new ? OJMS_DELETE : OJMS_MODIFY;
}
Class diagram for updated ovsdb_relay_run (runtime assertions)classDiagram
class relay_ctx {
+db
+cs
}
class ovsdb_relay_run {
+ovs_assert(ctx->db && ctx->cs)
+ovsdb_txn_forward_run(ctx->db, ctx->cs)
+ovsdb_cs_run(ctx->cs, &events)
}
ovsdb_relay_run ..> relay_ctx : uses
Class diagram for updated ovsdb_trigger_try (runtime assertion on reply)classDiagram
class ovsdb_trigger {
+request
+reply
}
class ovsdb_trigger_try {
+ovs_assert(t->reply)
+json_array_add(t->reply->result, ...)
+ovsdb_trigger_complete(t)
}
ovsdb_trigger_try ..> ovsdb_trigger : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds thread-safety annotations and locking in ofproto table description/status paths, adjusts initial row-update typing in OVSDB monitor, introduces a NULL-safe eid write in Raft, and inserts assertions in relay, transaction, and trigger code paths. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Ofproto
participant ofproto_mutex as Mutex
Client->>Ofproto: query_tables_desc()
Ofproto->>ofproto_mutex: lock()
Note right of Ofproto: Build table_desc snapshot
Ofproto-->>Client: table_desc[]
Ofproto->>ofproto_mutex: unlock()
Client->>Ofproto: send_table_status()
Ofproto->>ofproto_mutex: lock()
Note right of Ofproto: Read consistent vacancy/status
Ofproto-->>Client: status event
Ofproto->>ofproto_mutex: unlock()
sequenceDiagram
autonumber
participant Caller
participant Raft
Caller->>Raft: raft_next_entry(eid?)
alt eid pointer provided
Note over Raft: Assign *eid = entry.eid
else no eid pointer
Note over Raft: Skip eid write
end
Raft-->>Caller: serialized entry data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Ensure send_table_status is only called under ofproto_mutex or update its callers to acquire the mutex, since the OVS_REQUIRES annotation itself doesn’t enforce runtime locking.
- Consider replacing critical ovs_assert() checks (in relay, transaction, and trigger) with explicit null-pointer checks and error handling, as asserts may be disabled in production builds.
- Verify that all added OVS_REQUIRES annotations have corresponding lock acquisitions on every call path to avoid race conditions and satisfy static analysis.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Ensure send_table_status is only called under ofproto_mutex or update its callers to acquire the mutex, since the OVS_REQUIRES annotation itself doesn’t enforce runtime locking.
- Consider replacing critical ovs_assert() checks (in relay, transaction, and trigger) with explicit null-pointer checks and error handling, as asserts may be disabled in production builds.
- Verify that all added OVS_REQUIRES annotations have corresponding lock acquisitions on every call path to avoid race conditions and satisfy static analysis.
## Individual Comments
### Comment 1
<location> `ovsdb/transaction.c:649` </location>
<code_context>
}
if (ovsdb_base_type_is_weak_ref(&column->type.value)) {
+ ovs_assert(!datum->n || datum->values);
for (i = 0; i < datum->n; i++) {
find_and_add_weak_ref(src, &datum->keys[i], &datum->values[i],
</code_context>
<issue_to_address>
The assertion on datum->values may not catch all invalid states.
Also assert that datum->keys is non-null when datum->n is nonzero, since both arrays should be valid together.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| if (ovsdb_base_type_is_weak_ref(&column->type.value)) { | ||
| ovs_assert(!datum->n || datum->values); |
There was a problem hiding this comment.
suggestion: The assertion on datum->values may not catch all invalid states.
Also assert that datum->keys is non-null when datum->n is nonzero, since both arrays should be valid together.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ovsdb/relay.c (1)
383-383: Assert is fine; consider non-fatal guard in production loop.The assert prevents UB if ctx->db or ctx->cs is NULL. Optionally prefer a continue-with-log to avoid aborting the whole relay loop.
Apply this diff to degrade to a logged skip:
- ovs_assert(ctx->db && ctx->cs); + if (OVS_UNLIKELY(!ctx->db || !ctx->cs)) { + VLOG_ERR("%s: relay ctx missing db or cs", node->name); + continue; + }ovsdb/transaction.c (1)
649-649: Good invariant: prevents NULL deref on values when n > 0.Assertion aligns with value-typed weak-ref semantics and stops latent crashes.
If you prefer non-assert diagnostics in release, gate with OVS_UNLIKELY and log an OVSDB_BUG, then return early from the caller path.
ovsdb/trigger.c (1)
401-401: Defensive: assert reply before appending permanent error.Matches state machine expectations and guards a rare NULL-path.
Optionally create an empty array reply if reply is unexpectedly NULL to keep running without assert abort.
ovsdb/raft.c (1)
3611-3614: Clarify nullableeidin function docsIn ovsdb/raft.c (around line 3611), update the comment to:
- * Stores the entry ID of the log entry in '*eid'. + * If 'eid' is non-NULL, stores the entry ID in '*eid'.In ovsdb/raft.h (above the raft_next_entry declaration), add a matching doc comment:
/* Returns the next log entry or snapshot from 'raft', or NULL if none left. * If 'eid' is non-NULL, stores the entry ID in '*eid'. */ struct json *raft_next_entry(struct raft *, struct uuid *eid);All existing call sites pass a non-NULL
eid, so no code changes are required.ofproto/ofproto.c (1)
4292-4299: Locking fix looks right; consider snapshotting n_tables and annotating function exclusion.
- Minor: snapshot ofproto->n_tables to a local and iterate on that to avoid analyzer noise about size/loop divergence.
- Optional: mark query_tables_desc with OVS_EXCLUDED(ofproto_mutex) since it manages the lock internally (consistent with other helpers in this file).
-static void -query_tables_desc(struct ofproto *ofproto, struct ofputil_table_desc **descp) +static void +query_tables_desc(struct ofproto *ofproto, struct ofputil_table_desc **descp) + OVS_EXCLUDED(ofproto_mutex) { - struct ofputil_table_desc *table_desc; - size_t i; + struct ofputil_table_desc *table_desc; + size_t i; + size_t n = ofproto->n_tables; - table_desc = *descp = xcalloc(ofproto->n_tables, sizeof *table_desc); + table_desc = *descp = xcalloc(n, sizeof *table_desc); ovs_mutex_lock(&ofproto_mutex); - for (i = 0; i < ofproto->n_tables; i++) { + for (i = 0; i < n; i++) { struct ofputil_table_desc *td = &table_desc[i]; query_table_desc__(td, ofproto, i); } ovs_mutex_unlock(&ofproto_mutex); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
ofproto/ofproto.c(3 hunks)ovsdb/monitor.c(1 hunks)ovsdb/raft.c(1 hunks)ovsdb/relay.c(1 hunks)ovsdb/transaction.c(1 hunks)ovsdb/trigger.c(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: linux gcc check-offloads -100
- GitHub Check: linux gcc test -ljemalloc
- GitHub Check: linux gcc dpdk-shared --enable-shared
- GitHub Check: linux clang --enable-afxdp
- GitHub Check: linux clang dpdk-shared --enable-shared
- GitHub Check: linux gcc c99
- GitHub Check: linux clang dpdk-shared
- GitHub Check: linux gcc test --enable-shared
- GitHub Check: linux clang --disable-ssl
- GitHub Check: linux clang dpdk --enable-shared
- GitHub Check: linux gcc --disable-ssl
- GitHub Check: linux clang check check-dpdk dpdk
- GitHub Check: linux clang test --enable-shared
- GitHub Check: linux gcc test
- GitHub Check: linux clang c99
- GitHub Check: clang-analyze-cache
- GitHub Check: linux deb no dpdk
- GitHub Check: osx clang --disable-ssl
- GitHub Check: build oss-fuzz fuzzers
- GitHub Check: linux rpm fedora
🔇 Additional comments (4)
ovsdb/monitor.c (1)
684-688: Fixes initial update typing; avoids NULL ‘new’ deref in update2 path.Returning OJMS_INITIAL only when ‘new’ exists is correct and prevents compose_row_update2 from touching NULL ‘new’.
ovsdb/raft.c (1)
3625-3627: Good null-guard to prevent potential NPD in raft_next_entry().Writing to eid only when non-NULL eliminates a possible null deref without changing semantics for existing callers.
ofproto/ofproto.c (2)
4266-4269: Good: require ofproto_mutex for table-desc reads.Adding OVS_REQUIRES(ofproto_mutex) to query_table_desc__ matches how it's invoked and prevents racy reads of table fields. LGTM.
4330-4332: Good: enforce lock on vacancy/status path.OVS_REQUIRES(ofproto_mutex) on send_table_status aligns with all call sites (e.g., add_flow_finish, delete_flows_finish__). LGTM.
ovsrobot
pushed a commit
that referenced
this pull request
Sep 19, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
ovsrobot
pushed a commit
that referenced
this pull request
Oct 9, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Reproduce steps:
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs test -- set interface test type=internal
ip netns add ns_test
ip link set test netns ns_test
ip netns del ns_test
ifconfig br-ovs up
sleep 1
ifconfig br-ovs down
sleep 1
ovs-vsctl del-br br-ovs
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI Testing - sample trigger
[skip ci]
Summary by Sourcery
Address Coverity-reported issues by adding missing mutex locks and annotations, introducing null checks and correcting logic paths, and adding runtime assertions to enforce invariants across ofproto and ovsdb modules.
Bug Fixes:
Enhancements:
Summary by CodeRabbit