-
Notifications
You must be signed in to change notification settings - Fork 273
Limit class loading #604
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
Limit class loading #604
Conversation
64b5643
to
346d57e
Compare
What's the advantage of that in comparison to lazy class loading (#407)? |
It's in addition to lazy loading. If I understand correctly, lazy loading will only load the parts that are actually used and not everything that is present. This allows you to limit beforehand what is seen from a jar file, @marek-trtik had asked for such a feature. |
I see. These two are orthogonal. |
cegis needs json.a to compile: https://travis-ci.org/diffblue/cbmc/jobs/208614294#L2370 |
fb4917f
to
8e96c2a
Compare
added json.a to cegis Makefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Various requests for cleanup and logic simplifications.
src/java_bytecode/jar_file.cpp
Outdated
#include <sstream> | ||
|
||
#include <json/json_parser.h> | ||
#include <unordered_set> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit picking: group STL headers, only then start with local headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/java_bytecode/jar_file.cpp
Outdated
#include "jar_file.h" | ||
|
||
#include <util/suffix.h> | ||
#include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is iostream really required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/java_bytecode/jar_file.cpp
Outdated
get_message_handler(), | ||
json_cp_config)) | ||
throw "cannot read JSON input configuration for JAR loading"; | ||
assert(json_cp_config.is_object() && "JSON has wrong format"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an internal error (it's invalid user input), I think - should be throw instead of assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/java_bytecode/jar_file.cpp
Outdated
throw "cannot read JSON input configuration for JAR loading"; | ||
assert(json_cp_config.is_object() && "JSON has wrong format"); | ||
jsont include_files=json_cp_config["classFiles"]; | ||
assert(include_files.is_array() && "JSON has wrong format"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above: invalid user input, not an internal error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/java_bytecode/jar_file.cpp
Outdated
add_file|=std::regex_match(file_name, string_matcher, regex_matcher); | ||
// load .class file only if it is in the match set | ||
else | ||
add_file|=set_matcher.count(file_name)>0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use count() instead of find?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it was shorter, also it is an unordered map so it shouldn't be less efficient as count can only be 0 or 1 and if an element is found, the implemention likely stops traversing the unordered map. In any case, I change this to find()
to be consistent with the code base.
src/java_bytecode/jar_file.h
Outdated
jar_filet &operator()(const std::string &file_name) | ||
{ | ||
assert(!java_cp_include_files.empty() && "class regexp cannot be empty"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a user input, I believe?! Should be thrown, not asserted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
file_mapt::iterator it=file_map.find(file_name); | ||
if(it==file_map.end()) | ||
{ | ||
jar_filet &jar_file=file_map[file_name]; | ||
jar_file.open(file_name); | ||
jar_file.set_message_handler(get_message_handler()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does jar_filet benefit from being a messaget
(see comments above)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a status output to see about hidden files, also the JSON parser needs a valid one
@@ -55,6 +56,29 @@ void java_bytecode_languaget::get_language_options(const cmdlinet &cmd) | |||
lazy_methods_mode=LAZY_METHODS_MODE_CONTEXT_INSENSITIVE; | |||
else | |||
lazy_methods_mode=LAZY_METHODS_MODE_EAGER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a blank line. Codewithoutblanksisreallyhardtoread.
get_message_handler(), | ||
json_cp_config)) | ||
throw "cannot read JSON input configuration for JAR loading"; | ||
assert(json_cp_config.is_object() && "JSON has wrong format"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above for why I believe these aren't internal errors.
@@ -20,6 +20,11 @@ class java_class_loadert:public messaget | |||
{ | |||
public: | |||
java_bytecode_parse_treet &operator()(const irep_idt &); | |||
void set_java_cp_include_files(std::string &java_cp_include_files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A blank line, please.
e.g. ``` { "jar": [ "A.jar", "B.jar" ], "classFiles": [ "jarfile3$A.class", "jarfile3.class" ] } ```
8e96c2a
to
1975394
Compare
eae1b8b
to
53fad70
Compare
…p_on_exception [SEC-677] Turning DI off by default and making errors fail pipeline
This PR adds the capability to load only the subset of class files from a jar which match a regex specified via the
java-cp-include-files
parameter.requires #548