search

status-im / nim-metrics

Nim metrics client library supporting the Prometheus monitoring toolkit, StatsD and Carbon
Other
39 stars 6 forks source link

`--mm:orc` memory corruption #79

Open etan-status opened 6 months ago

etan-status commented 6 months ago

With --mm:orc in Nim 2.0.2, the {.gcsafe.} override in inc seems to corrupt labelValues memory.

template inc*(
    counter: Counter | type IgnoredCollector,
    amount: int64 | float64 = 1,
    labelValues: openArray[string] = [],
) =
  when defined(metrics) and counter is not IgnoredCollector:
    {.gcsafe.}:
      incCounter(counter, amount.float64, labelValues)

Observed on libp2p in lpchannel.nim where this is called from an {.async.} proc:

libp2p_protocols_bytes.inc(msgLen.int64, labelValues=[s.protocol, "out"])

Passed are ["/ipfs/id/1.0.0", "out"] values, by the time it hits the metrics internal newCounterMetrics helper, it becomes ["/ipipipipipipi", "out"], and on the way there goes through ["[\"[\\\"[\\\\\\\"[\\\\\\", "out"] before eventually SIGSEGV-ing.

With --mm:refc, all's good.

But maybe, accessing globals from procs that are marked as {.gcsafe.} is no longer practical with --mm:orc.

arnetheduck commented 6 months ago

metrics is indeed full of threading landmines that need to be cleared - none of the gcsafe here are actually gcsafe but rather work by accident more than anything else - afair it's linked due to how label values are moved across threads but there could be more to it as well.