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

database.cc: possible undefined behaviour during lister::scan_dir() call #2071

Closed
vladzcloudius opened this issue Feb 9, 2017 · 0 comments
Closed

Comments

@vladzcloudius
Copy link
Contributor

vladzcloudius commented Feb 9, 2017

Installation details
Scylla version: 9e4ae07

Description
lister::guarantee_type() assumes that engine().file_type() always returns engaged optional value. However this is a wrong assumption since it may return a not-engaged optional value if fstat() for the corresponding file failed with errno set to ENOENT or to ENOTDIR. This may happen for example due to some file system or disk error.

The rest of the code assumes that type value is engaged and uses operator*() on it without additional checks and this may lead to undefined behaviour.

In addition, the walker callbacks may behave unexpectedly too because in this case the condition:

if (de.type == directory_entry_type::xxx)
...

will always return false.

avikivity added a commit that referenced this issue Mar 6, 2017
"Work on this series started with fixing the  'nodetool clearsnapshot'.
The current master code  ignores the snapshots in deleted keyspaces (issue #2045).

I noticed that in many places our code has to build the path to some directory/file
it simply had the sstring(<path1>) + "/" + sstring(<path2>) constructs which may cause us issues
if somebody decides to complile/run scylla on not-Unix-based OS, like Microsoft Windows.

I understand that this is a long shot but if we can make it right now - why not to.
The answer is boost::filesystem::path class - its synchronous parts, of course.

I decided to take an initiative and fix the issues above and then use the fixed code for
fixing the issue #2045:
   - Fix some minor issues in the existing code.
   - Extend the lister class and move it into the separate files outside database.cc.

On the way I've found an issue in the existing code (issue #2071).
This series fixes this one too (PATCH2)."
benipeled pushed a commit to benipeled/scylla that referenced this issue May 31, 2022
Add a background fiber that works to free memory using spare cycles, so that
allocations don't have to evict cache synchronously. The shares for the fiber
are increased the closer we are to running out of memory, preferring to steal
cycles from the workload rather than encountering stalls.

The last patch is not strictly related but is a good idea.

See backport notes in the first patch. The others were trivial.

Test: unit (dev)

Ref scylladb#2113
Ref scylladb#2106
Ref scylladb#2071
Ref scylladb#2039

Closes scylladb#2129

* github.com:scylladb/scylla-enterprise:
  lsa: Mark compact_segment_locked() as noexcept
  lsa: Avoid excessive eviction if region is not compactible
  logalloc: fix quadratic behaviour of reclaim_from_evictable
  logalloc: reduce minimum lsa reserve in allocating_section to 1
  main: start background reclaim before bootstrap
  Merge 'lsa: background reclaim' from Avi Kivity
  logalloc: background reclaim
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

No branches or pull requests

1 participant