Fix integer overflows and truncation #278

stoeckmann · 2024-02-26T20:29:12Z

Make sure that calculations do not overflow.
Also do not truncate u64 to usize on 32 bit systems.

I have added tests to showcase the issues.

If tempfile is used on a 32 bit system, usize is u32. This means that casting u64 to usize may truncate the value. Test added to clarify the issue.

Stebalien

Overall, this makes the code much harder to read (except the first fix) for a correctness issue that's entirely theoretical (except in the first case). Did you run into any actual issues?

Stebalien · 2024-02-26T21:02:41Z

src/spooled.rs

@@ -96,7 +96,7 @@ impl SpooledTempFile {
    }

    pub fn set_len(&mut self, size: u64) -> Result<(), io::Error> {
-        if size as usize > self.max_size {
+        if size > self.max_size as u64 {


This one is definitely an improvement.

Stebalien · 2024-02-26T21:03:17Z

src/spooled.rs

+            if cursor
+                .position()
+                .checked_add(buf.len() as u64)
+                .filter(|&val| val <= self.max_size as u64)
+                .is_none()


This one isn't necessary (would require more addressable memory than a usize).

You could seek up to usize::MAX before calling write, then it will overflow. I haven't added a test case for this, because it could lead to a flush.

In fact, the test is easy. Added to commit for reference.

Ah, I see. You can seek past the end of the file. Yeah, you're right. And that's definitely something that can happen in practice.

Can we rewrite it with saturating add? I.e.: cursor.position.saturating_add(buf.len() as u64) > self.max_size as u64.

Yes, fortunately we can. I've had the false assumption that it would be an issue if self.max_size itself is usize::MAX but in this case, the underlying write calls will fail anyway.

Thanks for feedback so these is_none-checks can be removed from both places!

Stebalien · 2024-02-26T21:03:57Z

src/spooled.rs

+            if bufs
+                .iter()
+                .try_fold(cursor.position(), |a, b| a.checked_add(b.len() as u64))
+                .filter(|&val| val <= self.max_size as u64)
+                .is_none()


I guess this may be possible if someone tried to repeatedly write the same large slice over and over? But then they likely have other issues.

Ok, how about we write this entire check with saturating add? The filter + is_none is... going to trip me up eventually.

Yes, disliked this way of writing it as well. Fortunately it can be simplified as suggested. Done.

Stebalien · 2024-02-26T21:04:38Z

src/util.rs

+    let capacity = prefix
+        .len()
+        .checked_add(suffix.len())
+        .and_then(|value| value.checked_add(rand_len))
+        .ok_or(io::Error::new(io::ErrorKind::InvalidInput, "name too long"))?;


This is kind of pointless as the user would have to pass an insanely large filename (which would already cause issues).

Technically, the rand_len just has to be large enough, which just requires a .rand_bytes(usize::MAX) call. No need for a huge allocation of a filename beforehand.

But yes, you could argue that it's a user error to call that function with such a large value.

(well, it'll likely run out of memory and panic if the length is anywhere near usize::MAX)

But... how about we just use saturating math here as well (this is just for capacity). It avoids the error case and, and avoids the and_then etc.

Yeah, why not? I wanted to avoid the panic, but if someone really wants to try to allocate THAT much or just more than available, standard Rust will run into a panic anyway. The user asked for trouble doing that. :)

Adjusted as well. And removed the test.

stoeckmann · 2024-02-26T21:10:30Z

Did you run into any actual issues?

No, it's been about making the API robust against such values.

I've split the commits into two, since I assumed that it might be argued this way. So... Fell free to cherry pick, otherwise I can drop the second one.

Stebalien

Ok, you're right. We might as well fix these. But see my comments on using saturating add instead.

Stebalien · 2024-02-26T21:25:54Z

src/spooled.rs

+            if cursor
+                .position()
+                .checked_add(buf.len() as u64)
+                .filter(|&val| val <= self.max_size as u64)
+                .is_none()


Ah, I see. You can seek past the end of the file. Yeah, you're right. And that's definitely something that can happen in practice.

Stebalien · 2024-02-26T21:28:27Z

src/spooled.rs

+            if bufs
+                .iter()
+                .try_fold(cursor.position(), |a, b| a.checked_add(b.len() as u64))
+                .filter(|&val| val <= self.max_size as u64)
+                .is_none()


Ok, how about we write this entire check with saturating add? The filter + is_none is... going to trip me up eventually.

Stebalien · 2024-02-26T21:29:23Z

src/spooled.rs

+            if cursor
+                .position()
+                .checked_add(buf.len() as u64)
+                .filter(|&val| val <= self.max_size as u64)
+                .is_none()


Can we rewrite it with saturating add? I.e.: cursor.position.saturating_add(buf.len() as u64) > self.max_size as u64.

Stebalien · 2024-02-26T21:33:01Z

src/util.rs

+    let capacity = prefix
+        .len()
+        .checked_add(suffix.len())
+        .and_then(|value| value.checked_add(rand_len))
+        .ok_or(io::Error::new(io::ErrorKind::InvalidInput, "name too long"))?;


(well, it'll likely run out of memory and panic if the length is anywhere near usize::MAX)

But... how about we just use saturating math here as well (this is just for capacity). It avoids the error case and, and avoids the and_then etc.

Passing huge values might lead to integer overflows during calculations. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>

Stebalien

Thanks for the fixes and for working with me to find a good solution.

Stebalien · 2024-02-26T22:34:27Z

Published as v3.10.1.

stoeckmann · 2024-02-26T22:37:41Z

Thanks for the fixes and for working with me to find a good solution.

Thank you as well for the feedback and especially for maintaining the crate!

fix: handle size truncation on 32 bit systems

61a813b

If tempfile is used on a 32 bit system, usize is u32. This means that casting u64 to usize may truncate the value. Test added to clarify the issue.

stoeckmann changed the title ~~Fix integer overflow and truncation~~ Fix integer overflows and truncation Feb 26, 2024

Stebalien reviewed Feb 26, 2024

View reviewed changes

stoeckmann force-pushed the overflow branch from acbcf30 to c131a77 Compare February 26, 2024 21:22

Stebalien reviewed Feb 26, 2024

View reviewed changes

stoeckmann force-pushed the overflow branch from c131a77 to 5ac7750 Compare February 26, 2024 21:45

fix: handle integer overflows

aa06096

Passing huge values might lead to integer overflows during calculations. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>

stoeckmann force-pushed the overflow branch from 5ac7750 to aa06096 Compare February 26, 2024 21:55

Stebalien approved these changes Feb 26, 2024

View reviewed changes

Stebalien merged commit 56c5934 into Stebalien:master Feb 26, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix integer overflows and truncation #278

Fix integer overflows and truncation #278

stoeckmann commented Feb 26, 2024

Stebalien left a comment

Stebalien Feb 26, 2024

Stebalien Feb 26, 2024

stoeckmann Feb 26, 2024

stoeckmann Feb 26, 2024

Stebalien Feb 26, 2024

Stebalien Feb 26, 2024

stoeckmann Feb 26, 2024 •

edited

Stebalien Feb 26, 2024

Stebalien Feb 26, 2024

stoeckmann Feb 26, 2024

Stebalien Feb 26, 2024

stoeckmann Feb 26, 2024 •

edited

Stebalien Feb 26, 2024

stoeckmann Feb 26, 2024

stoeckmann commented Feb 26, 2024

Stebalien left a comment

Stebalien Feb 26, 2024

Stebalien Feb 26, 2024

Stebalien Feb 26, 2024

Stebalien Feb 26, 2024

Stebalien left a comment

Stebalien commented Feb 26, 2024

stoeckmann commented Feb 26, 2024

Fix integer overflows and truncation #278

Fix integer overflows and truncation #278

Conversation

stoeckmann commented Feb 26, 2024

Stebalien left a 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

stoeckmann Feb 26, 2024 • edited

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

stoeckmann Feb 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoeckmann commented Feb 26, 2024

Stebalien left a 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

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien commented Feb 26, 2024

stoeckmann commented Feb 26, 2024

stoeckmann Feb 26, 2024 •

edited

stoeckmann Feb 26, 2024 •

edited