open-telemetry / open-telemetry/opentelemetry-cpp

[BUG] ETW Properties::to_vector doubles the result and reads past a string_view

Open Beginner friendly
#4,347 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
Dominant language
C++
Stars
1.4k
Forks
632
Avg merge
1d 13h
Merged PRs (30d)
75

Description

Properties::to_vector(span<string_view>) in exporters/etw/include/opentelemetry/exporters/etw/etw_properties.h:94 has two defects on adjacent lines:

std::vector<std::string> static to_vector(const nostd::span<const nostd::string_view> &source)
{
  std::vector<std::string> result(source.size());
  for (const auto &item : source)
  {
    result.push_back(std::string(item.data()));
  }
  return result;
}

The result is twice the size it should be. std::vector<std::string> result(source.size()) value-initialises source.size() empty strings, and the loop then appends source.size() more. A span of three views comes back as six entries whose first three are empty.

std::string(item.data()) reads until a NUL. The function's own comment says the input is a span of "non-owning string views", and nostd::string_view::data() is not required to point at a NUL terminated buffer, so this can read past the view and copy whatever follows it.

The generic overload immediately above, at line 84, is correct and does neither:

template <typename T>
static std::vector<T> to_vector(const nostd::span<const T, nostd::dynamic_extent> &source)
{
  return std::vector<T>(source.begin(), source.end());
}

so the string_view overload looks like a copy that diverged.

It is live code. PropertyVariant::operator= calls it at line 260 for AttributeType::kTypeSpanString, so any ETW span or log record carrying a string-array attribute goes through it.

Why this is a report rather than a pull request

No CI job builds the ETW exporter. WITH_ETW appears in no workflow and in no branch of ci/do_ci.sh; the only workflow that names ETW at all is cppcheck.yml, which analyses rather than compiles. A fix here cannot be verified by the pipeline, and the to_vector change is entangled with the sizing bug rather than being a one line correction, so I would rather not push a blind change into a Windows-only path.

The suggested shape, if a maintainer wants it:

std::vector<std::string> result;
result.reserve(source.size());
for (const auto &item : source)
{
  result.push_back(static_cast<std::string>(item));
}
return result;

nostd::string_view has an explicit operator std::string() that copies by length.

I am happy to send that, but it would be more useful alongside a job that compiles the ETW exporter, since the sizing bug is the kind a single assertion would have caught. Found while sweeping the same string_view::data() shape for #4346, which fixes the four occurrences in components that CI does build.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read exporters/etw/include/opentelemetry/exporters/etw/etw_properties.h around the generic overload at line 84 and the string_view overload at line 94, then inspect PropertyVariant::operator= around line 260. Verify the ETW exporter on a span of string views, ensuring the returned vector has the same length and preserves non-NUL-terminated view contents; note that no CI job currently builds ETW.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
observability-sre
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.