[SERVER-55381] StringData find and rfind incorrect results Created: 20/Mar/21  Updated: 29/Oct/23  Resolved: 24/Mar/21

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: 5.0.0-rc0

Type: Bug Priority: Major - P3
Reporter: Billy Donahue Assignee: Billy Donahue
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-43583 StringData should get most or all str... Backlog
is related to SERVER-55180 Remove StringData <=> std::string_vie... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Service Arch 2021-03-22, Service Arch 2021-04-05
Participants:

 Description   

std::string and std::string_view have a precise definition for find which specifies what should happen when searching for an empty substring.

http://eel.is/c++draft/string.find
http://eel.is/c++draft/string.view#find
https://en.cppreference.com/w/cpp/string/basic_string/find

This confusion is the reason SERVER-55180 was reverted.
https://github.com/mongodb/mongo/blob/388ce132e9edd64cbfdf677a87ac5044ca3a79ed/src/mongo/db/exec/sbe/vm/vm.cpp#L1978
Changing a value's type from std::string_view to StringData changed the meaning of find.

If a StringData member function has the same name as a std::string or std::string_view member function, it should do exactly the same thing.

StringData::find:
https://github.com/mongodb/mongo/blob/master/src/mongo/base/string_data.h#L283

if (needleSize == 0)
        return 0;

Even if the needleSize is 0, we started looking at position pos so returning zero is incorrect. An empty string is considered found at the leftmost valid position in the search range.

Although StringData::find(substr,pos) takes 2 arguments, none of the tests for StringData ever specify the pos argument, and this function was essentially untested.

Fortunately this bug has no effect unless the pos is given, and very few callers give the pos argument.

There appear to be only two call sites that give the pos argument:

src/mongo/util/net/ssl_util.cpp|44 col 34| size_t headerPosition = blob.find(header, position);
src/mongo/util/net/ssl_util.cpp|54 col 35| size_t trailerPosition = blob.find(trailer, headerPosition);

And these two callers are assured that the header and trailer substrings are not empty because they were constructed locally.



 Comments   
Comment by Githook User [ 23/Mar/21 ]

Author:

{'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}

Message: SERVER-55381 fix StringData::find,rfind
Branch: master
https://github.com/mongodb/mongo/commit/0e7996afb9d63af16d4c3824e4d2698285f51f30

Comment by Billy Donahue [ 22/Mar/21 ]

After running some tests
( https://github.com/mongodb/mongo/commit/941652c7a5cbf957e0af9799fccf9a8d73dc7477 )
I have bad news about StringData::rfind, too!

{"t":{"$date":"2021-03-22T06:34:06.839Z"},"s":"E",  "c":"TEST",     "id":23070,   "ctx":"main","msg":"Throwing exception","attr":{"exception":"Expected withStdString == withStringData (0 == 18446744073709551615) s:'foo', ch:'f', pos:0 @src/mongo/base/string_data_test.cpp:276"}}

behavior of rfind is specified by: http://eel.is/c++draft/string.view.find#6

Comment by Billy Donahue [ 22/Mar/21 ]

CR https://mongodbcr.appspot.com/764610001

Generated at Thu Feb 08 05:36:18 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.