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

Custom regex parallel verify #1127

Merged
merged 3 commits into from
Feb 28, 2023
Merged

Custom regex parallel verify #1127

merged 3 commits into from
Feb 28, 2023

Conversation

0x1
Copy link
Contributor

@0x1 0x1 commented Feb 24, 2023

No description provided.

@0x1 0x1 requested a review from a team as a code owner February 24, 2023 17:21
Copy link
Collaborator

@ahrav ahrav left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -82,66 +83,98 @@ func (c *customRegexWebhook) FromData(ctx context.Context, verify bool, data []b
// ]
matches := permutateMatches(regexMatches)

g := new(errgroup.Group)
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Are we okay not setting an upper bound on the number of goroutines here? Is the idea the len of matches will be low enough for this not to cause issues?

If we did plan on bounding the number of goroutines we can add:
g.SetLimit(<some-limit>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcastorina thoughts?

Copy link
Collaborator

@mcastorina mcastorina Feb 24, 2023

Choose a reason for hiding this comment

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

I was thinking unlimited would be okay because it's at most 100 goroutines that will be doing "async I/O" work.

pkg/custom_detectors/custom_detectors.go Outdated Show resolved Hide resolved
if err := g.Wait(); err != nil {
return nil, err
}
close(resultsCh)
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: I think we should move this up right after you created the chan and defer the close. Otherwise if we encounter an err in the goroutines w/in the errgroup we are going to leak with this unclosed channel.

so just: defer close(resultsCh) at L90

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aaah! yes that's way cleaner!!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately we need to close the channel before iterating over it otherwise the loop on line 103 would never end.

I do think that maybe we can ignore the error returned by g.Wait() though, and collect as many results that we got.

@0x1 0x1 merged commit 2315192 into main Feb 28, 2023
@0x1 0x1 deleted the custom-regex-parallel-verify branch February 28, 2023 16:12
javajawa added a commit to mewbotorg/mewbot that referenced this pull request Mar 6, 2023
Bump trufflesecurity/trufflehog from 3.28.2 to 3.28.7.

v3.28.7
What's Changed
Support filtering detectors by version by @​mcastorina in trufflesecurity/trufflehog#1150
Full Changelog: trufflesecurity/trufflehog@v3.28.6...v3.28.7

v3.28.6
What's Changed
Rename .pre-commit-hooks.yml to .pre-commit-hooks.yaml by @​zhuwenxing in trufflesecurity/trufflehog#1141
Keyword optimization by @​zricethezav in trufflesecurity/trufflehog#1144
Release should only run on tags by @​dustin-decker in trufflesecurity/trufflehog#1146
New Contributors
@​zhuwenxing made their first contribution in trufflesecurity/trufflehog#1141
@​zricethezav made their first contribution in trufflesecurity/trufflehog#1144
Full Changelog: trufflesecurity/trufflehog@v3.28.5...v3.28.6

v3.28.5
What's Changed
[chore] - Only scanned staged git changes by @​ahrav in trufflesecurity/trufflehog#1143
Full Changelog: trufflesecurity/trufflehog@v3.28.4...v3.28.5

v3.28.4
What's Changed
[chore] Address more linter errors by @​mcastorina in trufflesecurity/trufflehog#1134
Custom regex parallel verify by @​0x1 in trufflesecurity/trufflehog#1127
[chore] Close response bodies by @​mcastorina in trufflesecurity/trufflehog#1137
Bump github.com/stretchr/testify from 1.8.1 to 1.8.2 by @​dependabot in trufflesecurity/trufflehog#1130
Add pre-commit yml config by @​ahrav in trufflesecurity/trufflehog#1138
Disable profiler in debug mode and add profile switch by @​yilmi in trufflesecurity/trufflehog#1136
Full Changelog: trufflesecurity/trufflehog@v3.28.3...v3.28.4

v3.28.3
What's Changed
Support file scanning in filesystem source by @​mcastorina in trufflesecurity/trufflehog#1030
Add ability to include and exclude detectors by @​mcastorina in trufflesecurity/trufflehog#1106
[chore] Implement String for ScanErrors by @​mcastorina in trufflesecurity/trufflehog#1131
[chore] Update docs for individual file scanning by @​mcastorina in trufflesecurity/trufflehog#1132
[chore] Address lint errors by @​mcastorina in trufflesecurity/trufflehog#1133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants