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

Fix parse_hex algorithm #1267

Closed
wants to merge 1 commit into from
Closed

Conversation

compmaniak
Copy link
Contributor

Current parse_hex implementation doesn't work at the extreme case when input is "fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff...". The function consumes all of such chars but returns maximum value for the specified type. This happens because of bounds checking via integer overflow.

@codecov
Copy link

codecov bot commented Oct 15, 2018

Codecov Report

Merging #1267 into develop will decrease coverage by <.01%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1267      +/-   ##
===========================================
- Coverage    95.43%   95.43%   -0.01%     
===========================================
  Files          112      112              
  Lines        11072    11074       +2     
===========================================
+ Hits         10567    10568       +1     
- Misses         505      506       +1
Impacted Files Coverage Δ
include/boost/beast/http/detail/basic_parser.hpp 89.88% <90%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec1c8ad...dca5f3e. Read the comment docs.

@vinniefalco
Copy link
Member

Good find! I will add a test to confirm. Can you please give me more details? What is the exact string, is it "fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"? How many f are in there? And what is the "specified type" you are using?

@vinniefalco
Copy link
Member

I understand the problem now. Good work

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