[GitHub] [madlib] reductionista commented on a change in pull request #560: Strengthen security checks on compile params

[GitBox <gi...@apache.org>]

reductionista commented on a change in pull request #560:
URL: https://github.com/apache/madlib/pull/560#discussion_r590934148



##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_wrapper.py_in
##########
@@ -440,16 +444,16 @@ def get_custom_functions_list(compile_params):
 
     """
     compile_dict = convert_string_of_args_to_dict(compile_params)
-    builtin_losses = dir(losses)
+    builtin_losses = update_builtin_losses(dir(losses))
     builtin_metrics = update_builtin_metrics(dir(metrics))
 
     custom_fn_list = []
     local_loss = compile_dict['loss'].lower() if 'loss' in compile_dict else None
-    local_metric = compile_dict['metrics'].lower()[2:-2] if 'metrics' in compile_dict else None
+    metric_list = ast.literal_eval(compile_dict['metrics'].lower()) if 'metrics' in compile_dict else []

Review comment:
       I think it's better to catch the problem on master, then send bad input to the segments and let them find out.  
   
   I see nothing wrong with calling literal eval twice--in fact, I think it's best we interpret the string they pass in the same way each time we process it.  If it's handled one way here, and then another way later, then there are more possibilities for how the user can insert quotes and other special characters to confuse MADlib.
   
   However, looking at this again I notice there is a try catch block around the literal_eval that happens on the segments.  If it errors out here, it won't give as nice a message... so maybe we should add a similar try/catch around it?




----------------------------------------------------------------
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