Skip to content

Add extra checks for baggage header parsing #1010

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

Merged
merged 2 commits into from
May 2, 2025

Conversation

songxiaosheng
Copy link
Contributor

@songxiaosheng songxiaosheng commented Mar 20, 2025

image

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Mar 20, 2025

Thank you very much for the PR!

Could you please confirm that no ArrayIndexOutOfBoundsExeption is thrown from this component but you see a log event on DEBUG level instead?

Since key-values must be in pairs (the length of that array must be even), I'm not sure we should only fix this like you did and silently parse something that can be invalid. I think we should also let the user know that what they are doing is wrong. Maybe by doing something like this (without throwing an exception, just logging the issue): KeyValues.

I think we should do three things:

  1. Apply your fix for index bounds
  2. Length check to let the user know that the data is invalid
  3. Add some tests
  4. Do this on the 1.3.x branch so that all the supported versions will be fixed

What do you think? Are you up for doing these changes?
Also, in order to accept your changes, you should sign-off the commit, please see the DCO check failing.

/cc @marcingrzejszczak

@jonatan-ivanov jonatan-ivanov added the bug A general bug label Mar 20, 2025
@jonatan-ivanov jonatan-ivanov added this to the 1.3.11 milestone Mar 20, 2025
@marcingrzejszczak
Copy link
Contributor

Ah yeah we haven't actually considered any invalid cases

@songxiaosheng
Copy link
Contributor Author

Thank you for your guidance. I will try to experience a better and more complete implementation

Thank you very much for the PR!

Could you please confirm that no ArrayIndexOutOfBoundsExeption is thrown from this component but you see a log event on DEBUG level instead?

Since key-values must be in pairs (the length of that array must be even), I'm not sure we should only fix this like you did and silently parse something that can be invalid. I think we should also let the user know that what they are doing is wrong. Maybe by doing something like this (without throwing an exception, just logging the issue): KeyValues.

I think we should do three things:

  1. Apply your fix for index bounds
  2. Length check to let the user know that the data is invalid
  3. Add some tests
  4. Do this on the 1.3.x branch so that all the supported versions will be fixed

What do you think? Are you up for doing these changes?
Also, in order to accept your changes, you should sign-off the commit, please see the DCO check failing.

/cc @marcingrzejszczak

@jonatan-ivanov
Copy link
Member

Please let us know if you need any help.

@songxiaosheng
Copy link
Contributor Author

Please let us know if you need any help.

Thank you, you can help review again after I resubmit the PR

@shakuzen shakuzen modified the milestones: 1.3.11, 1.3.x Apr 14, 2025
@jonatan-ivanov jonatan-ivanov changed the base branch from main to 1.3.x May 2, 2025 19:45
@jonatan-ivanov
Copy link
Member

I rebased this on 1.13.x, added a length check, added a debug log to let the user know that the data is invalid and added tests. Since your change was trivial, I set the DCO check to pass manually.

@jonatan-ivanov jonatan-ivanov changed the title avoid IndexOutOfBoundsException Add extra checks for baggage header parsing May 2, 2025
@jonatan-ivanov jonatan-ivanov merged commit 3f2f9ba into micrometer-metrics:1.3.x May 2, 2025
7 checks passed
@jonatan-ivanov jonatan-ivanov modified the milestones: 1.3.x, 1.3.12 May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants