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

Only create SourceFile for leak on demand #4021

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Sep 15, 2023

Timing each LeakPattern and the creation of SourceFile separately in a repo with a single 150MB file of random bytes revealed this:

AWS Access Key: 190 ms
AWS Secret Key: 1434 ms
Google API Key: 143 ms
Google OAuth ID: 337 ms
Google OAuth Refresh Token: 244 ms
Google OAuth Access Token: 97 ms
Private Key: 169 ms
RSA Private Key: 156 ms
EC Private Key: 179 ms
PGP Private Key: 227 ms
creating source files: 45777 ms

With this change, leak detection for big files will look like:

Publishing _dummy_pkg 0.0.1-toobig to https://pub.dev:
├── CHANGELOG.md (<1 KB)
├── LICENSE (<1 KB)
├── README.md (<1 KB)
├── pubspec.yaml (<1 KB)
└── testfile (146 MB)
Validating package... (14.3s)
Package validation found the following errors:
* Potential leak of Private Key in testfile at offset 153600000:153600106.
  
  ```
  -----BEGIN PRIVATE KEY-----
  H0M6xpM2q+53wmsN/eYLdgtjgBd3DBmHtPilCkiFICXyaA8z9LkJ
  -----END PRIVATE KEY-----
  ```
  
  Add a git-ignore style pattern to `false_secrets` in `pubspec.yaml`
  to ignore this. See https://dart.dev/go/false-secrets

@sigurdm sigurdm requested a review from jonasfj September 15, 2023 08:14
@sigurdm sigurdm changed the title Only create SourceFile for leak when detected Only create SourceFile for leak on demand Sep 26, 2023
Comment on lines 118 to 122
SourceSpan span() {
final sourceFile =
sourceFileCache[url] ??= SourceFile.fromString(content, url: url);
return sourceFile.span(start, end);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing around sourceFileCache like this is an ugly hack.

Just remove the span() function. This isn't used many places.. Then have the place the prints LeakMatch instances do your hack. If that's what you want.


It might also be reasonable not to compute SourceFile when the file is sufficient large.

It might be reasonable to detect binary files and not do SourceSpan for such files.


Or maybe a factory constructor on SourceFile that returns a _LazySourceFile which lazily computes the line numbers.

class _LazySourceFile implements SourceFile {
  final Iterable<int> _decodedChars;
  final Object? _url;
  late final _sourceFile = SourceFile.decoded(_decodedChars, _url);

  _LazySourecFile(this._decodedChars, this._url) {
    // TODO: Some cheap validations of _url and _decodedChars, just handle cases where
    //       SourceFile.decoded would throw.
  }

  @override
  int get lines => _sourceFile.lines;

  // TODO: All other overrides...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the cache, and now only create the sourcefile when "describe"ing the leak.

That way we might create sourcefile multiple times for the same file, but as we at most display 3 leaks it should not be a problem.

I also implemented a limit on when we are creating the sourcefile. Above that we don't attempt to display line numbers, but just offsets.

@sigurdm sigurdm requested a review from jonasfj September 27, 2023 14:07
@sigurdm sigurdm merged commit ea54dab into dart-lang:master Sep 27, 2023
23 checks passed
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

Successfully merging this pull request may close these issues.

2 participants