Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EXPORTER] Do not use regex in CleanUpString because some implementations of STL may crash. #2464

Merged
merged 4 commits into from
Dec 23, 2023

Conversation

owent
Copy link
Member

@owent owent commented Dec 20, 2023

Fixes #2463

Changes

  • Remove the usage of regex in PrometheusExporterUtils::CleanUpString
    • I didn't tested but it should have better performance than using std::regex_replace, because it allocate less std::string objects.
  • Use explicit std::string constructor in std::regex_replace because some versions of gcc can not detect the types correctly.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@owent owent requested a review from a team December 20, 2023 09:03
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0dd5eed) 87.04% compared to head (c231c82) 87.06%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2464      +/-   ##
==========================================
+ Coverage   87.04%   87.06%   +0.02%     
==========================================
  Files         199      199              
  Lines        6087     6087              
==========================================
+ Hits         5298     5299       +1     
+ Misses        789      788       -1     

see 1 file with indirect coverage changes

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a known issue with gcc and regexp:

// Regex support
#if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8 || __GNUC_MINOR__ == 9))
#  define OPENTELEMETRY_HAVE_WORKING_REGEX 0
#else
#  define OPENTELEMETRY_HAVE_WORKING_REGEX 1
#endif

The code in exporter_utils.cc did not check for OPENTELEMETRY_HAVE_WORKING_REGEX, probably causing the bug found.

About the fix,

For PrometheusExporterUtils::CleanUpString, I totally agree that using 4 regexp in a row was overkill, and replacing regexp with custom code is much better.

About PrometheusExporterUtils::RemoveUnitPortionInBraces,
leaving the code as is with the regexp will still fail in gcc 4.8 and 4.9.

Could the fix also reimplement RemoveUnitPortionInBraces(), to remove the usage of regex completely ?

@owent
Copy link
Member Author

owent commented Dec 22, 2023

There is a known issue with gcc and regexp:

// Regex support
#if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8 || __GNUC_MINOR__ == 9))
#  define OPENTELEMETRY_HAVE_WORKING_REGEX 0
#else
#  define OPENTELEMETRY_HAVE_WORKING_REGEX 1
#endif

The code in exporter_utils.cc did not check for OPENTELEMETRY_HAVE_WORKING_REGEX, probably causing the bug found.

About the fix,

For PrometheusExporterUtils::CleanUpString, I totally agree that using 4 regexp in a row was overkill, and replacing regexp with custom code is much better.

About PrometheusExporterUtils::RemoveUnitPortionInBraces, leaving the code as is with the regexp will still fail in gcc 4.8 and 4.9.

Could the fix also reimplement RemoveUnitPortionInBraces(), to remove the usage of regex completely ?

Thanks, it use OPENTELEMETRY_HAVE_WORKING_REGEX to decide whether to use the fallback implantation now, and I also add a fallback implantation for PrometheusExporterUtils::RemoveUnitPortionInBraces. This function won't crash but it have a wrong result before, sorry for the missing.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

@marcalff marcalff merged commit ddfafff into open-telemetry:main Dec 23, 2023
46 checks passed
@marcalff marcalff changed the title Do not use regex in CleanUpString because some implementations of STL may crash. [EXPORTER] Do not use regex in CleanUpString because some implementations of STL may crash. Dec 23, 2023
@owent owent deleted the fixes_2463 branch January 10, 2024 02:25
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.

Some old STL implementation of regex have BUGs and will crash.
3 participants