Repository navigation
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe series query baton now places ChangesSeries query baton
Suggested reviewers: Priority: ➖ Normal Change: Bug fix Merge Risk: ⚪ Minimal · up to The baton layout and tracing changes show no introduced merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. A rabbit checks the baton’s shape Comment |
Error being reported by UBSAN:
query.c:98:11: runtime error: member access within misaligned address 0x000000000007 for type 'struct node_t', which requires 8 byte alignment
0x000000000007: note: pointer points here
<memory cannot be printed>
#0 0x7f61e117e896 in skip_free_value_set [...]/src/libpcp_web/src/query.c:98
performancecopilot#1 0x7f61e117eb1f in freeSeriesQueryNode [...]/src/libpcp_web/src/query.c:114
performancecopilot#2 0x7f61e1180926 in freeSeriesGetQuery [...]/src/libpcp_web/src/query.c:154
performancecopilot#3 0x7f61e11822f9 in series_query_finished [...]/src/libpcp_web/src/query.c:284
performancecopilot#4 0x7f61e11827dc in series_query_end_phase [...]/src/libpcp_web/src/query.c:303
performancecopilot#5 0x7f61e120551e in key_lookup_mapping_callback [...]/src/libpcp_web/src/query.c:6138
performancecopilot#6 0x7f61e1248eac in keySlotsReplyCallback [...]/src/libpcp_web/src/slots.c:590
performancecopilot#7 0x7f61e12d95a3 in valkeyClusterAsyncCallback deps/libvalkey/src/cluster.c:3051
performancecopilot#8 0x7f61e12c1473 in valkeyRunCallback deps/libvalkey/src/async.c:300
performancecopilot#9 0x7f61e12c219c in valkeyAsyncFreeInternal deps/libvalkey/src/async.c:349
performancecopilot#10 0x7f61e1247db4 in valkeyLibuvPoll ../../../vendor/github.com/valkey-io/libvalkey/include/valkey/adapters/libuv.h:54
performancecopilot#11 0x7f61e1d8154c in uv__io_poll /usr/src/debug/libuv-1.52.1-1.el10_2.x86_64/src/unix/linux.c:1546
performancecopilot#12 0x7f61e1d6b9f1 in uv_run /usr/src/debug/libuv-1.52.1-1.el10_2.x86_64/src/unix/core.c:460
performancecopilot#13 0x55c5271c9217 in main_loop [...]/src/pmproxy/src/server.c:921
performancecopilot#14 0x55c5271c823f in main [...]/src/pmproxy/src/pmproxy.c:473
performancecopilot#15 0x7f61dfe5158d in __libc_start_call_main (/lib64/libc.so.6+0x2a58d) (BuildId: cc6959d636202b0e1d6e17b273bf4060ec9265e5)
performancecopilot#16 0x7f61dfe51648 in __libc_start_main_alias_2 (/lib64/libc.so.6+0x2a648) (BuildId: cc6959d636202b0e1d6e17b273bf4060ec9265e5)
performancecopilot#17 0x55c5271c8c34 in _start ([...]/src/pmproxy/src/pmproxy+0x65c34) (BuildId: 8145839e69ef9e1eeb443adc33bf70cf083cf8ba)
Root cause:
seriesGetLookup (which contains seriesGetSID series[], a flexible array
member) was placed before seriesGetQuery query in seriesQueryBaton:
typedef struct seriesQueryBaton {
seriesBatonMagic header; /* MAGIC_QUERY */
seriesBatonPhase *current;
seriesBatonPhase phases[QUERY_PHASES];
pmSeriesModule *module;
pmSeriesCallBacks *callbacks;
pmLogInfoCallBack info;
void *userdata;
keySlots *slots;
int error;
seriesGetLookup lookup;
seriesGetQuery query;
} seriesQueryBaton;
This cause overwriting baton->query.root with sid->header.magic = 7 when
calling initSeriesBatonMagic(sid, MAGIC_SID).
Trigger:
When the valkey connection dropped, the error path in
series_query_end_phase called series_query_finished → freeSeriesGetQuery
on a lookup baton, which then passed the corrupted SeriesBaton magic
(0x7) to be passed to freeSeriesQueryNode.
Fix:
Because seriesGetLookup is a flexible array, it has to be last in
structure, therefore seriesGetQuery and seriesGetLookup must be swapped.
Reproducer:
In grafana open the PCP Valkey: Metric Preview, set to 5s refresh, then
continuously start/stop the valkey service:
# while :; do systemctl stop valkey; echo "valkey stopped"; sleep 3.1; \
systemctl start valkey; echo "valkey started"; sleep 10.1; done
After some time (unpredictable), the connection may break just after a
Lookup Baton has been referenced, causing freeSeriesGetQuery to be freed
instead. This was verified with PCP_DEBUG=desperate level and new
instrumentation on line 218; pmproxy.log then shows the issue on Baton
0x51500007c680:
Baton [query/0x51500007c680] verified magic - 6 (@ series_lookup_services)
Baton [query/0x51500007c680] references: 0 -> 1 (@ series_lookup_services)
Baton [query/0x51500007c680] verified magic - 6 (@ series_query_end_phase)
Baton [query/0x51500007c680] references: 1 -> 0 (@ series_query_end_phase[seriesPassBaton])
Baton [query/0x51500007c680] verified magic - 6 (@ series_lookup_mapping)
Baton [query/0x51500007c680] 0 references - verified (@ series_lookup_mapping)
HERE BELOW Lookup Baton is referenced
Baton [query/0x51500007c680] references: 0 -> 1 (@ series_lookup_mapping)
:
[Fri Oct 2 13:13:28] pmproxy(375872) Error: Lost connection to key server.
:
Baton [query/0x51500007c680] verified magic - 6 (@ key_lookup_mapping_callback)
Baton [query/0x51500007c680] verified magic - 6 (@ series_query_end_phase)
HERE BELOW Query Baton is dereferenced while it should be Lookup Baton!!
Baton [query/0x51500007c680] references: 1 -> 0 (@ series_query_end_phase)
Baton [query/0x51500007c680] verified magic - 6 (@ freeSeriesGetQuery)
Baton [query/0x51500007c680] 0 references - verified (@ freeSeriesGetQuery)
query.c:98:11: runtime error: member access within misaligned address 0x000000000007 for type 'struct node_t', which requires 8 byte alignment
[...]
Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
Pull Request Description
Error being reported by UBSAN:
Root cause:
seriesGetLookup(which containsseriesGetSID series[], a flexible array member) was placed beforeseriesGetQuery queryinseriesQueryBatonstructure:This cause overwriting
baton->query.rootwithsid->header.magic = 7when callinginitSeriesBatonMagic(sid, MAGIC_SID).Trigger:
When the valkey connection dropped, the error path in
series_query_end_phasecalledseries_query_finished→freeSeriesGetQueryon a lookup baton, which then passed the corruptedSeriesBatonmagic (0x7) to be passed tofreeSeriesQueryNode.Fix:
Because
seriesGetLookupis a flexible array, it has to be last in structure, thereforeseriesGetQueryandseriesGetLookupmust be swapped.Reproducer:
In grafana open the
PCP Valkey: Metric Preview, set to 5s refresh, then continuously start/stop the valkey service:After some time (unpredictable), the connection may break just after a Lookup Baton has been referenced, causing
freeSeriesGetQueryto be freed instead. This was verified withPCP_DEBUG=desperatelevel and new instrumentation on line 218;pmproxy.logthen shows the issue on Baton 0x51500007c680:Related PR
See also ASAN+UBSAN PR