You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by GitBox <gi...@apache.org> on 2020/09/11 10:33:36 UTC
[GitHub] [systemds] sebwrede opened a new pull request #1054: [MINOR] Remove Exceptions From FederatedResponse
sebwrede opened a new pull request #1054:
URL: https://github.com/apache/systemds/pull/1054
Exceptions added to the FederatedResponse risk exposing data from the federated worker. Exceptions need to be caught and then a new exception could be created and added to the FederatedResponse without all details from the original exception.
An example of how to add an exception to the FederatedResponse without exposing data can be seen in "executeCommand" in FederatedWorkerHandler.java:
`catch (DMLPrivacyException | FederatedWorkerHandlerException ex) {return new FederatedResponse(ResponseType.ERROR, ex);}`
This code catches DMLPrivacyExceptions and FederatedWorkerHandlerExceptions and any other type of exception is handled by a different catch-block where the name of the exception class is put into the message of a FederatedWorkerHandlerException. In this way, we can throw DMLPrivacyExceptions and FederatedWorkerHandlerExceptions without including any private data in the exception message and all other exceptions will not be returned in the FederatedResponse.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] sebwrede commented on a change in pull request #1054: [MINOR] Remove Exceptions From FederatedResponse
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #1054:
URL: https://github.com/apache/systemds/pull/1054#discussion_r487786099
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedWorkerHandler.java
##########
@@ -155,7 +155,7 @@ private FederatedResponse executeCommand(FederatedRequest request) {
catch (Exception ex) {
return new FederatedResponse(ResponseType.ERROR,
new FederatedWorkerHandlerException("Exception of type "
- + ex.getClass() + " thrown when processing request", ex));
+ + ex.getClass() + " thrown when processing request"));
Review comment:
We need to send the DMLPrivacyExceptions since we want the coordinator to understand why a FederatedRequest was rejected by the worker. In that way, it would later be possible for the coordinator to mitigate the problem by sending a different request which does not violate the privacy constraints.
The DMLPrivacyException class is only used when checking privacy constraints, hence we control which information is put in the instances of the class. This means that we can safely return the exception to the coordinator without worrying about exposing private data. The same idea applies to the FederatedWorkerHandlerException: we create instances of this class and we are therefore able to control the information in the exception instances.
This is different from the other exceptions because they could be thrown from anywhere, be of any exception type, and contain any kind of information. This means that it is safer to catch such exceptions and return our own exception type with a message we control without including the original exception in the federated response.
We could add the catch clause for DMLPrivacyExceptions in other place, but generally I try to throw exceptions early and catch them late. If I catch DMLPrivacyExceptions in other places it should be to rethrow them and only catch them in executeCommand so that we only create the FederatedResponse once instead of having that logic in several different places. Also, the DMLPrivacyException is only thrown in some places which means that it should never be caught in for instance the "execClear" method. However, we can still add it so that the exception is caught if the code is changed to throw the privacy exception in the future.
I have changed the code now so that it has the catch clause for DMLPrivacyException and FederatedWorkerHandlerException before the Exception catch clauses.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] asfgit closed pull request #1054: [MINOR] Remove Exceptions From FederatedResponse
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1054:
URL: https://github.com/apache/systemds/pull/1054
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] mboehm7 commented on pull request #1054: [MINOR] Remove Exceptions From FederatedResponse
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on pull request #1054:
URL: https://github.com/apache/systemds/pull/1054#issuecomment-753058524
LGTM - thanks for the detailed reasoning. I only added log error messages at the federated site to allow for debugging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] mboehm7 commented on a change in pull request #1054: [MINOR] Remove Exceptions From FederatedResponse
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on a change in pull request #1054:
URL: https://github.com/apache/systemds/pull/1054#discussion_r487525755
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedWorkerHandler.java
##########
@@ -155,7 +155,7 @@ private FederatedResponse executeCommand(FederatedRequest request) {
catch (Exception ex) {
return new FederatedResponse(ResponseType.ERROR,
new FederatedWorkerHandlerException("Exception of type "
- + ex.getClass() + " thrown when processing request", ex));
+ + ex.getClass() + " thrown when processing request"));
Review comment:
Could you elaborate why we send the exception for privacy exceptions but NOT for all other exceptions? Shouldn't it be the other way around? In that spirit I would also add the catch clause for PrivacyExceptions in the other places, while in general send the exceptions to allow for debugging at the coordinator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org