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

Add malicious sites from polish cert. #2121

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Conversation

utoni
Copy link
Collaborator

@utoni utoni commented Oct 27, 2023

  • care about parsing errors for *.list

@utoni utoni force-pushed the add/malicious-sites branch 6 times, most recently from f6bf68f to bc05fad Compare October 27, 2023 16:38
@utoni utoni requested a review from IvanNardi October 29, 2023 10:13
Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

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

While the PR is fine per se (and the error handling is great) I have a "philosophical" doubt:
do we want to start adding lists with ~150K entries to the library?
I know that these lists are somehow optional and any users might skip them...
I had similar doubts with the gambling list, when it increased from the original some K entries to 30k entries...

Just some thoughts... @lucaderi, what do you think?

example/ndpiReader.c Outdated Show resolved Hide resolved
@IvanNardi
Copy link
Collaborator

While the PR is fine per se (and the error handling is great) I have a "philosophical" doubt: do we want to start adding lists with ~150K entries to the library? I know that these lists are somehow optional and any users might skip them... I had similar doubts with the gambling list, when it increased from the original some K entries to 30k entries...

After some thoughts, I am fine with the list.
I am still not sure about how to handle the performance issue, though

@utoni
Copy link
Collaborator Author

utoni commented Oct 30, 2023

A "manual-lists-download" script would solve checking-in large files to git (and reduce init-time if no list was downloaded), but then the user needs to download those and needs to know about such script.

@IvanNardi
Copy link
Collaborator

Ok, let keep it as you proposed... Could you please change tests/ossfuzz.sh and add a simple trace with a session matching one of these domains, please?

 * added handling of parsing errors

Signed-off-by: Toni Uhlig <[email protected]>
Copy link

sonarcloud bot commented Nov 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

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

Thanks!

@IvanNardi IvanNardi merged commit 6dcecd7 into ntop:dev Nov 2, 2023
33 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