Sanitize AppendVec's file_size #7373

ryoqun · 2019-12-09T08:39:30Z

Problem

Creation and restoration of AppendVec is not secure with regard to size of mmap.
Currently, validators can easily be panic!-ed just by putting empty (0-sized) appendvec file or very large sparse appendvec file. The former is forbidden by the specs of mmap and the later will cause panics depending on system configuration/load.

Summary of Changes

cap the maximum mmaped appendvec size to 16GiB. This effectively limits the maximum account data size to roughly 8GiB (half of 16GB according to current AppendVec capacity reservation policy) with plenty of margin to the hotly debated limit on the individual account sizes (ref: put limit on account data length #7320).
minor runtime behavior change and code cleanup along wide the above.

Part of #7167

ryoqun · 2019-12-09T08:47:39Z

runtime/src/append_vec.rs

@@ -21,6 +21,8 @@ macro_rules! align_up {
    };
 }

+const MAXIMUM_APPEND_VEC_FILE_SIZE: usize = 16 * 1024 * 1024 * 1024; // 16 GiB


Could be much lower considering it seems AccountsDB never increases its .file_size from DEFAULT_FILE_SIZE.

It doesn't increase, it may allocate a larger one than default if the store account is larger than the default file size.

Thanks for confirmation, then maybe we could lower to be close in relation to #7320.

For now, I think this is fine as it is now because we can assume the running system will have at least 16GB RAM as per the doc and we can be relieved we'll never be tripped by the limit at that high number depending on current AccountDB's behavior and its possible future changes.

Or, could this be lower, say, a half?: 8GB

runtime/src/append_vec.rs

ryoqun · 2019-12-09T08:50:17Z

runtime/src/append_vec.rs

    fn visit_bytes<E>(self, data: &[u8]) -> std::result::Result<Self::Value, E>
    where
        E: serde::de::Error,
    {
        use serde::de::Error;
        let mut rd = Cursor::new(&data[..]);
        let current_len: usize = deserialize_from(&mut rd).map_err(Error::custom)?;
-        let map = MmapMut::map_anon(1).map_err(|e| Error::custom(e.to_string()))?;
-        Ok(AppendVec {


Moved this logic to newly created fn new_empty_map. I can't live with this code. ;)

ryoqun · 2019-12-09T08:52:57Z

runtime/src/append_vec.rs

-            map,
-            append_offset: Mutex::new(current_len),
-            current_len: AtomicUsize::new(current_len),
-            file_size: current_len as u64,


NOTE: I changed this behavior because this looked just a wrong thing. New code started to initialize this correctly with actual file size.

Or is this intentional to make appencvecs read-only?

ryoqun · 2019-12-09T08:54:36Z

runtime/src/append_vec.rs

@@ -451,6 +490,47 @@ pub mod tests {
        }
    }

+    #[test]
+    #[should_panic(expected = "too small file size 0 for AppendVec")]


I know those should_panics are rather ugly. I'll brush up once this PR's direction is affirmed.

Other uses are fixed by now. Only this will remain as this is testing a panic from ::new().

ryoqun · 2019-12-09T08:55:51Z

runtime/src/append_vec.rs

+    fn sanitize_mmap_size(size: usize) -> io::Result<()> {
+        if size == 0 {
+            Err(std::io::Error::new(
+                std::io::ErrorKind::Other,


I know these Errors are not welcomed as explicitly mentioned by #6446.

ryoqun · 2019-12-09T09:01:24Z

runtime/src/append_vec.rs

@@ -174,14 +208,27 @@ impl AppendVec {

    #[allow(clippy::mutex_atomic)]
    pub fn set_file<P: AsRef<Path>>(&mut self, path: P) -> io::Result<()> {
-        self.path = path.as_ref().to_path_buf();


Only update self.path in an atomic way if everything went successful here.

ryoqun · 2019-12-09T09:04:50Z

runtime/src/append_vec.rs

        let map = unsafe { MmapMut::map_mut(&data)? };
+
+        self.file_size = file_size;


Update file_size correctly, will be used later PRs to check the sum of contained accounts doesn't exceed this and moreover exactly matches to the current_len.

Update file_size correctly, will be used later PRs to check the sum of contained acconts doesn't exceed this and moreover exactly matches to the current_len.

As promised by 2 weeks ago's myself, this is done by this and this.

codecov · 2019-12-09T09:05:20Z

Codecov Report

Merging #7373 into master will decrease coverage by 9.8%.
The diff coverage is 64.4%.

@@           Coverage Diff            @@
##           master   #7373     +/-   ##
========================================
- Coverage    68.7%   58.9%   -9.9%     
========================================
  Files         244     244             
  Lines       56028   64972   +8944     
========================================
- Hits        38530   38309    -221     
- Misses      17498   26663   +9165

ryoqun · 2019-12-09T09:08:24Z

runtime/src/append_vec.rs

@@ -96,6 +98,8 @@ impl AppendVec {
            }
        }

+        AppendVec::sanitize_mmap_size(size).unwrap();


yes, unwrap()ing here just because considering this is under the supertubes milestone and there isn't enough time to do large refactoring (would mean Bank.store() starts to fail). There are many other security issues to be fixed for snapshot for the milesone.

I'm pretty sure this code (=AppendVec::new) isn't touched by the snapshot deserialization code path at all.

Move this above if create {

ryoqun · 2019-12-09T09:15:26Z

@sakridge @mvines Could you take a look? Of course, you can do in your spare time possibly after the ongoing super fun time has passed. :)

ryoqun · 2019-12-09T09:22:30Z

runtime/src/append_vec.rs

@@ -138,6 +143,35 @@ impl AppendVec {
        }
    }

+    #[allow(clippy::mutex_atomic)]
+    pub fn new_empty_map(current_len: usize) -> Self {
+        let map = MmapMut::map_anon(1).expect("failed to map the data file");


This error handling is made more critical rather undesirably from Err to just move the code here. But combined with this consideration, this can be tolerated to avoid large refactoring. :) And this mmap will almost never fail.

ryoqun · 2019-12-09T09:24:41Z

runtime/src/append_vec.rs

+        let file_size = std::fs::metadata(&path)?.len();
+        assert!(current_len <= file_size as usize);
+
+        AppendVec::sanitize_mmap_size(file_size as usize)?;


Notice the ?; Now we're correctly propagating this error up to the snapshot deserializing code.

runtime/src/append_vec.rs

sakridge · 2019-12-09T23:59:06Z

Overall it looks pretty good to me.

runtime/src/append_vec.rs

ryoqun · 2019-12-12T05:02:21Z

@mvines pinging for your first impression of this PR. Especially this introduces another fixed/constant cluster-wide-common hard limit in the snapshot area :)

stale · 2019-12-19T06:03:08Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

ryoqun · 2019-12-19T10:32:11Z

Another round of brush up and I think this PR isn't draft anymore with good confidence. :)

@sakridge @mvines Could you review this again for being merged?

runtime/src/append_vec.rs

stale · 2019-12-31T01:34:39Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

sakridge

lgtm

ryoqun added 4 commits December 9, 2019 16:22

Check append vec file size

0399a54

Don't use panic

bb6ad6c

Clean up a bit

a43bff4

Clean up

7845e68

ryoqun commented Dec 9, 2019

View reviewed changes

runtime/src/append_vec.rs Outdated Show resolved Hide resolved

ryoqun commented Dec 9, 2019

View reviewed changes

ryoqun changed the title ~~[wip] Sanitize append vec offset~~ [wip] Sanitize AppendVec's file_size Dec 9, 2019

ryoqun commented Dec 9, 2019

View reviewed changes

sakridge reviewed Dec 9, 2019

View reviewed changes

runtime/src/append_vec.rs Outdated Show resolved Hide resolved

ryoqun commented Dec 11, 2019

View reviewed changes

runtime/src/append_vec.rs Outdated Show resolved Hide resolved

This was referenced Dec 13, 2019

Strictly sanitize mmapped AppendVec file contents #7464

Merged

Cap file size for snapshot data files #7182

Merged

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 19, 2019

ryoqun added 4 commits December 19, 2019 19:10

Clean ups

ecd85cc

Change assertion into sanization check

62f68dd

Remove...

ba86ef5

Clean up

efefb39

ryoqun added 3 commits December 19, 2019 19:10

More clean up

a396f87

More clean up

4ada26b

Use assert_matches

4de2f14

stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 19, 2019

ryoqun changed the title ~~[wip] Sanitize AppendVec's file_size~~ Sanitize AppendVec's file_size Dec 19, 2019

ryoqun marked this pull request as ready for review December 19, 2019 10:30

ryoqun requested review from sakridge and mvines December 19, 2019 10:32

mvines reviewed Dec 19, 2019

View reviewed changes

runtime/src/append_vec.rs Show resolved Hide resolved

mvines approved these changes Dec 20, 2019

View reviewed changes

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 31, 2019

ryoqun removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 6, 2020

sakridge approved these changes Jan 6, 2020

View reviewed changes

ryoqun merged commit 58e6d4a into solana-labs:master Jan 6, 2020

ryoqun mentioned this pull request Jan 6, 2020

Fix AppendVec test breakage... #7693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitize AppendVec's file_size #7373

Sanitize AppendVec's file_size #7373

ryoqun commented Dec 9, 2019 •

edited

Loading

ryoqun Dec 9, 2019

sakridge Dec 9, 2019

ryoqun Dec 12, 2019

ryoqun Dec 24, 2019

ryoqun Dec 9, 2019

ryoqun Dec 9, 2019

ryoqun Dec 9, 2019

ryoqun Dec 19, 2019

ryoqun Dec 9, 2019

ryoqun Dec 9, 2019

ryoqun Dec 9, 2019 •

edited

Loading

ryoqun Dec 19, 2019 •

edited

Loading

codecov bot commented Dec 9, 2019 •

edited

Loading

ryoqun Dec 9, 2019 •

edited

Loading

ryoqun Dec 11, 2019

ryoqun commented Dec 9, 2019

ryoqun Dec 9, 2019

ryoqun Dec 9, 2019

sakridge commented Dec 9, 2019

ryoqun commented Dec 12, 2019

stale bot commented Dec 19, 2019

ryoqun commented Dec 19, 2019

stale bot commented Dec 31, 2019

sakridge left a comment

		let map = unsafe { MmapMut::map_mut(&data)? };

		self.file_size = file_size;

Sanitize AppendVec's file_size #7373

Sanitize AppendVec's file_size #7373

Conversation

ryoqun commented Dec 9, 2019 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

ryoqun Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2019 • edited Loading

Codecov Report

ryoqun Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakridge commented Dec 9, 2019

ryoqun commented Dec 12, 2019

stale bot commented Dec 19, 2019

ryoqun commented Dec 19, 2019

stale bot commented Dec 31, 2019

sakridge left a comment

Choose a reason for hiding this comment

ryoqun commented Dec 9, 2019 •

edited

Loading

ryoqun Dec 9, 2019 •

edited

Loading

ryoqun Dec 19, 2019 •

edited

Loading

codecov bot commented Dec 9, 2019 •

edited

Loading

ryoqun Dec 9, 2019 •

edited

Loading