- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3.1k
perf: optimize CSP nonce generation #14340
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
base: dev
Are you sure you want to change the base?
Conversation
Pre-allocate nonces Vec and reuse a single buffer with `write!` instead of `format!`, reducing heap allocations when replacing tokens.
| Package Changes Through 282ed06There are 1 changes which include tauri with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request. 
 Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector | 
This change introduces a performance optimization for Content Security Policy (CSP) nonce generation.
| let mut nonces = Vec::with_capacity(asset.matches(token).count()); | ||
| *asset = replace_with_callback(asset, token, || { | ||
| #[cfg(target_pointer_width = "64")] | ||
| let mut raw = [0u8; 8]; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasfernog do you remember why we used usize here at the first place (tracing back to cf54dcf)? It doesn't quite make sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I don't get the question, but the code is still using a usize there, just that it's using its underlying byte representation instead, right?
In any case, seems like this code can be simplified down to:
let mut raw = [0u8; std::mem::size_of::<usize>()];
getrandom::fill(&mut raw).expect("failed to get random bytes");
let nonce = usize::from_ne_bytes(raw);Or even:
let mut raw = 0_usize.to_ne_bytes();
getrandom::fill(&mut raw).expect("failed to get random bytes");
let nonce = usize::from_ne_bytes(raw);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant it doesn't make sense to make nonce length depends on target pointer size
| #[cfg(target_pointer_width = "32")] | ||
| let mut buf = String::with_capacity(20); | ||
| #[cfg(target_pointer_width = "16")] | ||
| let mut buf = String::with_capacity(14); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these capacities meant to be how many bytes to alloc to store 'nonce-{0..usize::MAX}'? If so, isn't it a bit oversized for 32 and 16 ptr widths? Feels like it should be 18 and 13 respectively, unless I'm missing something
If we want to be a bit fancy-pants (😛) we could use this helper function:
const fn nonce_source_capacity() -> usize {
    // 'nonce-' prefix + digits of usize::max + closing quote
    8 + usize::MAX.ilog10() as usize + 1
}(You can test that by replacing usize for u64, u32 and u16 respectively)
| buf.clear(); | ||
| write!(&mut buf, "'nonce-{}'", nonce).unwrap(); | ||
| sources.push(buf.clone()); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reusing buf doesn't seem helpful in this scenario, if we'll clone it anyway. It means that the code will allocate nonces.len()+1 times
for nonce in nonces {
      let mut buf = String::with_capacity(nonce_capacity());
      write!(&mut buf, "'nonce-{}'", nonce).unwrap();
      sources.push(buf);
}Something like that ^ would allocate n times rather than n+1 times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly doubt these optimizations will actually make any differences, format! Itself is quite optimized to estimate the buffer size already
Pre-allocate nonces Vec and reuse a single buffer with
write!instead offormat!, reducing heap allocations when replacing tokens.Adjust CSP nonce buffer size based on target pointer width