Fix for out of bound read in std::strtol while parsing HTTP requests

[alexprabhat99]

Fixes: #1193

[tyler92]

Is it possible to use more modern way?

https://en.cppreference.com/w/cpp/utility/from_chars

It requires C++17

[kiplingw]

Thanks for your contribution @alexprabhat99.

The std::strtol function really shouldn't be used in any new code. As @tyler92 already pointed out you're better off using std::from_chars which was introduced in C++ 17. Pistache supports C++17.

There are a number of advantages to using the newer std::from_chars. Unlike std::strtol and related, it is non-throwing, does everything it needs to do on the stack (non-allocating), which also means better memory safety, locale independent, better error reporting, and apparently has better performance.

I recommend refactoring your PR to use the std::from_chars in src/common/{http,http_header}.cc as well as in src/common/net.cc. Make sure you bump the patch version in version.txt per our documentation because you've changed the internal implementation, but not any interfaces.

@Tachi107, feel free to chime in.

[dgreatwood]

(Just commenting since I noticed I was referenced on the issue) I agree std::from_chars is a nice approach. There are a couple of tiny differences between strtol and from_chars: 1/ from_chars does not ignore leading white space, strtol does. 2/ from_chars does not recognize a leading "+" sign (or signs), whereas strtol treats a leading plus (or pluses) as indicating that the value is positive, which is the default in any case. For safety, you could move the cursor forward until it reaches a spot that is not whitespace, and is not a "+", ignoring those characters, or it reaches the end of the buffer. Then (presuming cursor has not reached buffer end) apply from_chars.

[alexprabhat99]

I sincerely apologize for the delay. @kiplingw I have made the required changes and incorporated @dgreatwood's points. This PR is now ready for review.

[kiplingw]

I sincerely apologize for the delay. @kiplingw I have made the required changes and incorporated @dgreatwood's points. This PR is now ready for review.

No worries @alexprabhat99. Thanks for contributing. I've left some comments above.

[alexprabhat99]

@kiplingw I have made the changes as per your review.

[kiplingw]

Thanks @alexprabhat99, but it looks like there are multiple failures in CI related to unit tests. I'm not sure if this is a result of your PR per se, but normally they don't all bail like that. Unfortunately I don't have time to go into a deep dive right now, but maybe @Tachi107 can take a look.

[dgreatwood]

Hi Alex -

Thanks for incorporating the prior feedback.

I can quickly point out what Kip is referring to. Of course, if this is already obvious you can disregard my comments :-)

pistache runs a number of github workflows (aka tests) upon git push, including the ones defined in .github/workflows/linux.yaml in the project. You can see the results of those tests either by going to your PR ( https://github.com/pistacheio/pistache/pull/1201) and scrolling down to where you see "Some checks were not successful", then click on "linux". Alternatively, at the top level of pistache, you can click on the "Actions" button, scroll down to and click on your PR, and click on the "linux" test.

Either way, once you've opened the "linux" Test, you should see the log of what happened. Searching the log for "FAIL" usually gives a pretty good clue what went wrong :-)

Hope this helps. D.

On Mon, Apr 22, 2024 at 3:01 PM Kip @.***> wrote:

Thanks @alexprabhat99 https://github.com/alexprabhat99, but it looks like there are multiple failures in CI related to unit tests. I'm not sure if this is a result of your PR per se, but normally they don't all bail like that. Unfortunately I don't have time to go into a deep dive right now, but maybe @Tachi107 https://github.com/Tachi107 can take a look.

— Reply to this email directly, view it on GitHub https://github.com/pistacheio/pistache/pull/1201#issuecomment-2071024935, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFMA252OPZI57FQ7P2JAW3Y6WCEXAVCNFSM6AAAAABGEUROI6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGAZDIOJTGU . You are receiving this because you were mentioned.Message ID: @.***>

-- NOTICE: This email and its attachments may contain privileged and confidential information, only for the viewing and use of the intended recipient. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, acting upon, or use of the information contained in this email and its attachments is strictly prohibited and that this email and its attachments must be immediately returned to the sender and deleted from your system. If you received this email erroneously, please notify the sender immediately.  Xage Security, Inc. and its affiliates will never request personal information (e.g., passwords, Social Security numbers) via email.  Report suspicious emails to @. @.>

[dgreatwood]

I also meant to mention that the debian:testing tests are broken anyway right now. But the others, such as debian:stable, should work once you've perfected your PR.

And of course you can run at least one form of the test suite prior to pushing to github using: meson test -C build (where build is your meson build directory)

Good luck.

On Mon, Apr 22, 2024 at 9:46 PM Duncan Greatwood @.***> wrote:

Hi Alex -

Thanks for incorporating the prior feedback.

I can quickly point out what Kip is referring to. Of course, if this is already obvious you can disregard my comments :-)

pistache runs a number of github workflows (aka tests) upon git push, including the ones defined in .github/workflows/linux.yaml in the project. You can see the results of those tests either by going to your PR ( https://github.com/pistacheio/pistache/pull/1201) and scrolling down to where you see "Some checks were not successful", then click on "linux". Alternatively, at the top level of pistache, you can click on the "Actions" button, scroll down to and click on your PR, and click on the "linux" test.

Either way, once you've opened the "linux" Test, you should see the log of what happened. Searching the log for "FAIL" usually gives a pretty good clue what went wrong :-)

Hope this helps. D.

On Mon, Apr 22, 2024 at 3:01 PM Kip @.***> wrote:

Thanks @alexprabhat99 https://github.com/alexprabhat99, but it looks like there are multiple failures in CI related to unit tests. I'm not sure if this is a result of your PR per se, but normally they don't all bail like that. Unfortunately I don't have time to go into a deep dive right now, but maybe @Tachi107 https://github.com/Tachi107 can take a look.

— Reply to this email directly, view it on GitHub https://github.com/pistacheio/pistache/pull/1201#issuecomment-2071024935, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFMA252OPZI57FQ7P2JAW3Y6WCEXAVCNFSM6AAAAABGEUROI6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGAZDIOJTGU . You are receiving this because you were mentioned.Message ID: @.***>

-- NOTICE: This email and its attachments may contain privileged and confidential information, only for the viewing and use of the intended recipient. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, acting upon, or use of the information contained in this email and its attachments is strictly prohibited and that this email and its attachments must be immediately returned to the sender and deleted from your system. If you received this email erroneously, please notify the sender immediately.  Xage Security, Inc. and its affiliates will never request personal information (e.g., passwords, Social Security numbers) via email.  Report suspicious emails to @. @.>