Add semgrep rule for .elapsed()
#4369
Conversation
| languages: | ||
| - rust | ||
| message: Use fedimint_core::time::now and std::time::duration_since to compare an elapsed time for better wasm compatibility. | ||
| pattern: $SYSTEM_TIME.elapsed() |
There was a problem hiding this comment.
I was hoping:
- id: ban-system-time-elapsed
languages:
- rust
message: TBD
pattern-either:
- pattern: std::time::SystemTime::elapsed
- pattern: std::time::Instant::elapsed
severity: WARNING
would work, but it seems it doesn't. I guess method calls need to be done via $foo.bar?
There was a problem hiding this comment.
Yeah, I was hoping the same and came to the same conclusion for method calls via $foo.bar.
It's also worth noting a footgun:
pattern: '($SYSTEM_TIME: std::time::SystemTime).elapsed()'
This is valid syntax for typed metavariables and implies it will only match method calls to SystemTime.elapsed(), however semgrep is unable to parse the type from struct fields so we could accidentally sneak in additional calls to .elapsed().
There was a problem hiding this comment.
Yeah, I guess semgrep would have to do a perfect type resolution and inference like rustc to be able to know types of method receivers, which seems prohibitively complex and slow for basically a linter.
| @@ -909,7 +909,7 @@ impl LightningClientModule { | |||
| .get_value(&MetaOverridesKey {}) | |||
| .await | |||
| { | |||
| let elapsed = now().duration_since(cache.fetched_at).unwrap(); | |||
| let elapsed = now().duration_since(cache.fetched_at)?; | |||
There was a problem hiding this comment.
Can we make this an unwrap_or_default to stay in sync with 0.2.3? See #4370
bradleystachurski
force-pushed
the
elapsed-semgrep
branch
from
0935007
to
e0551a0
Compare
|
What about just banning use of |
I think that's a great idea! |
Shoot, I like the idea but semgrep doesn't catch the import. This is a draft PR with a global ban of |
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
Followup to prevent further bugs similar to #4356
This rule catches usages of method calls to
.elapsed(). I did an audit of all public functions forstd::time::SystemTimeand this is the only one that callsstd::time::SystemTime::now.Semgrep supports typed metavariables (docs), which would be much better than catching any usage of
.elapsed(), however after testing locally it doesn't catch the usage in the associated PR. Rust support is beta and I believe it's limited in its ability to parse types from struct fields. We're currently using semgrep v1.37.0, so I installed the latest version and that doesn't work either.Semgrep PR introducing typed metavariables for rust: semgrep/semgrep#8201