Skip to content

Fix use-after-free of DataStorage metrics page address label#390

Open
vporoshok wants to merge 1 commit into
ppfrom
fix/metrics-page-uaf
Open

Fix use-after-free of DataStorage metrics page address label#390
vporoshok wants to merge 1 commit into
ppfrom
fix/metrics-page-uaf

Conversation

@vporoshok

Copy link
Copy Markdown
Collaborator

Summary

Fixes intermittent invalid (non-UTF-8) values for the address label of prompp_data_storage_* metrics, which made prompp's self-scrape fail to parse its own metrics (invalid utf8 in label_value ... prompp_data_storage_asc_int_count{address=...}).

Two related root causes:

  1. Dangling label view (use-after-free). The address label value was a non-owning Go::String view into a std::string stored inside DataStorage. On DataStorage destruction the metrics page was only detached (deferred removal), but the backing string was freed immediately in the destructor. A concurrent scrape that already advanced onto the page would read freed/reused memory — hence the rare garbage bytes. Ownership of the address string is moved into the Metrics page, so its lifetime matches the page and it is reclaimed only by remove_unused_pages().

  2. Unserialized scrapes. client_golang does not serialize Gather/Collect, so two concurrent scrapes could race: prompp_metrics_iterator_ctor calls remove_unused_pages() (which physically deletes detached pages), so one scrape could delete a page another scrape is still iterating. A mutex now serializes the whole CppMetrics iteration, guaranteeing the single-reader invariant the detach/remove design relies on.

Changes

  • pp/series_data/metrics.hMetrics page owns address_label_ (moved in); metrics built from a label_set() helper viewing the page-owned string.
  • pp/series_data/data_storage.h — drop the address_label_ member from DataStorage; pass the address string to the page at construction.
  • pp/go/cppbridge/metrics.go — package-level sync.Mutex held for the full CppMetrics iteration.
  • pp/series_data/tests/metrics_tests.cpp — regression test asserting the address label stays valid after the owning DataStorage is destroyed.

Test plan

  • make build-entrypoint (ARM devcontainer)
  • C++ //:metrics_test, //:series_data_test pass
  • New lifetime regression test passes, also under --asan=true
  • Go ./pp/go/cppbridge metrics tests pass
  • gofmt clean

Made with Cursor

Two related issues caused intermittent invalid (non-UTF-8) values for the
`address` label of `prompp_data_storage_*` metrics during self-scrape:

1. The `address` label value was a non-owning view (Go::String) into a
   std::string stored inside DataStorage. On DataStorage destruction the
   page was only detached, but the backing string was freed immediately, so
   a concurrent scrape holding the page read freed memory. Move ownership of
   the address string into the Metrics page so its lifetime matches the page
   (reclaimed only by remove_unused_pages()).

2. Metric scrapes were not serialized. client_golang does not serialize
   Gather/Collect, so two concurrent scrapes could race: one deletes (via
   remove_unused_pages) a page another is still iterating. Add a mutex around
   the whole CppMetrics iteration to guarantee a single reader.

Add a regression test asserting the address label stays valid after the
owning DataStorage is destroyed.

Co-authored-by: Cursor <cursoragent@cursor.com>
@gshigin gshigin requested review from cherep58 and gshigin June 23, 2026 07:44
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.

2 participants