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

Added support for connecting to LocalSocket (Unix Socket) files #21

Closed
wants to merge 1 commit into from

Conversation

hayes-roach
Copy link

No description provided.

@hayes-roach
Copy link
Author

@lokori @drogin @anttivi-solita Can I get someone to review this, please?

@drogin
Copy link
Contributor

drogin commented Feb 18, 2021

@hayes-roach I have added a PR for index out of bounds exception as well, which has been open for quite a while. I'm not sure if the authors/solita is actively maintaining this repository anymore. I have no authority here, although I have previously contributed a bit.

As for my feedback on the PR:
Two things makes me reluctant:

  1. The import of a code we don't fully understand/control. Maybe not much of an issue, but so far the project has been very minimal, with few dependencies and potential security flaws.
  2. To use UNIX Sockets, I presume the library will use JNI, which at least historically is considered slow.

And point 2 makes me ask, why do you need local sockets? ClamAV can be configured to use regular TCP/IP sockets and loopback/localhost interface. Is it because you want to avoid the IP-layer to gain performance? If so, I would benchmark if it actually is faster. My theory is that your code would gain speed due to UNIX Socket, but lose speed due to JNI. I'm unsure of the scale between the two, so I would benchmark

@hayes-roach
Copy link
Author

@drogin Thanks for the feedback, I understand the dependency situation as it could possibly bring in some security problems. My reasoning for adding UNIX Sockets is because of improved performance as well as security issues that can arise if the TCP/IP sockets are not configured correctly. We are currently using this repository with UNIX Sockets in some of our services.

@hayes-roach hayes-roach closed this by deleting the head repository Nov 5, 2024
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