fix: data race in Pistache::Rest::Router::route

[tyler92]

This patch fixes a data race in Pistache::Rest::Router::route that occurs when several HTTP requests are handled in parallel. Multiple threads were writing to the same std::unordered_map (routes) without synchronization. There is no need to modify the map because the method is inherently constant; it’s sufficient to find the target element in the map without adding a default value via operator[]. Additionally, the method has been marked as const to prevent this issue in the future.

Closes: #1238

[tyler92]

I'm not sure if I should increment the minor version in version.txt

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.11%. Comparing base (f71275b) to head (22e6822).
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
- Coverage   77.55%   76.11%   -1.44%     
==========================================
  Files          55       58       +3     
  Lines        7615     9614    +1999     
==========================================
+ Hits         5906     7318    +1412     
- Misses       1709     2296     +587     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kiplingw]

My guess is adding a cv qualifier would change the ABI if it was a public interface. If it is, then minor should be incremented. @Tachi107?

[Tachi107]

My guess is adding a cv qualifier would change the ABI if it was a public interface. If it is, then minor should be incremented. @Tachi107?

I believe it does, but in any case we don't currently follow such ABI scheme. The SONAME version is only set to the major component. I've proposed considering the minor too to ease ABI versioning in #1146, but it hasn't been merged yet.

If we merge it, then yes.

[dgreatwood]

Hi @tyler92 -

I had a look at this, and came back with a question.

1/ I agree it is an issue.

2/ It's good to make hasNotFoundHandler() marked const.

3/ It took me a minute to work out why the old code for "Router::route" was changing the map. Of course, as you have realized, the critical line is:
auto& r = routes[req.method()];
since std::unordered_map<...>::operator[] "Returns a reference to the value that is mapped to a key equivalent to key or x respectively, performing an insertion if such key does not already exist."

I was wondering how we know that the code does not rely on that insertion side effect, which you've removed in this PR? After all, the insertion would only happen if "routes" does not already have an entry for "req.method()" - maybe the insertion is actually needed?

[tyler92]

@dgreatwood Hi, thank you for the feedback.

The code works the same way with or without insertion, but as you mentioned, there is a side effect involving a default-constructed element in the map, which leads to a data race.

Having a default-constructed element in the map has the same semantics as not having it.

Additionally, the nature of the route method is not mutable; there is no reason to modify the map, as it only finds a handler that matches the incoming request and passes the request to it. This is a common misuse of operator[].

[dgreatwood]

@tyler92 -

Having a default-constructed element in the map has the same semantics as not having it.

Can we be confident that the code doesn't come along later and populate the initially-empty SegmentTreeNode that is inserted as part of the <Http::Method, SegmentTreeNode> insertion?

If so, then this looks an excellent change to me.

[tyler92]

Can we be confident that the code doesn't come along later

Yes, the const qualifier helps with it

[dgreatwood]

Sure, in that function, yes. But I was more meaning later on in some other function / code path?

[kiplingw]

On Thu, 2024-09-05 at 12:18 -0700, dgreatwood wrote: 3/ It took me a minute to work out why the old code for "Router::route" was changing the map. Of course, as you have realized, the critical line is: auto& r = routes[req.method()]; since std::unordered_map<...>::operator[] "Returns a reference to the value that is mapped to a key equivalent to key or x respectively, performing an insertion if such key does not already exist."

I wonder if changing the unordered_map usage to find() or count() instead could solve the const issue since neither affects its state?

…

-- Kip Warner OpenPGP signed/encrypted mail preferred https://www.thevertigo.com

[dgreatwood]

Ah yes. If I understand your comment correctly, that is exactly what @tyler92's PR does.

We're assuming that the function is accidentally non-const.

The thing I was suggesting we check was to make sure that it is not deliberately non-const. In particular, that another code path does not later rely on the <Http::Method, SegmentTreeNode> inserted by the [] operator.

[tyler92]

Probably we can ask @Tachi107 if the method is non-const intentionally or not but I believe it's not.

[kiplingw]

@dgreatwood, is this ready to merge?

[dgreatwood]

Please check the version number looks right, otherwise yes.

…

On Tue, Sep 10, 2024 at 4:28 PM Kip ***@***.***> wrote: @dgreatwood <https://github.com/dgreatwood>, is this ready to merge? — Reply to this email directly, view it on GitHub <#1239 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFMA27UNAU7ZBTPA6RHTNDZV56BRAVCNFSM6AAAAABNNZEPBGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBSGMZTENRUGU> . 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 ***@***.*** ***@***.***>