s-yata / marisa-trie

MARISA: Matching Algorithm with Recursively Implemented StorAge
Other
506 stars 89 forks source link

race condition in Mapper::open_() #36

Open setharnold opened 3 years ago

setharnold commented 3 years ago

Hello, I'm giving marisa a quick review as part of an Ubuntu Main Inclusion Request: https://bugs.launchpad.net/ubuntu/+source/marisa/+bug/1914808 This is just a quick review, nothing too deep.

Coverity reported a race condition in Mapper::open_():

https://github.com/s-yata/marisa-trie/blob/006020c1df76d0d7dc6118dacc22da64da2e35c4/lib/marisa/grimoire/io/mapper.cc#L139

If another process were to swap out the file between the stat() call and the open() call, the size might refer to a different file.

This probably isn't a big deal, since a process that could do that could do other silly things to break marisa, but I figured it's worth reporting all the same.

Coverity gives a lot of results, some look like mistakes in Coverity, some look like harmless issues (std::rand() use in tests, exceptions bubbling up to main() in tests, etc.). I'll attach the whole Coverity report to the Ubuntu bug, in case you're curious.

Thanks

lib/marisa/grimoire/io/mapper.cc:141:
  1. fs_check_call: Calling function "stat" to perform check on "filename".
lib/marisa/grimoire/io/mapper.cc:141:
  2. path: Condition "!(stat(filename, &st) != 0)", taking true branch.
lib/marisa/grimoire/io/mapper.cc:142:
  3. path: Condition "!((marisa::UInt64)st.st_size > 18446744073709551615UL /* (size_t)~((size_t)0) */)", taking true branch.
lib/marisa/grimoire/io/mapper.cc:145:
  4. toctou: Calling function "open" that uses "filename" after a check function. This can cause a time-of-check, time-of-use race condition.