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.

Sign up for GitHub

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

Jump to bottom

Constant attr to visitor #3540

Merged
merged 8 commits into from
Dec 23, 2020
Merged

Constant attr to visitor #3540

merged 8 commits into from
Dec 23, 2020

Conversation

pelszkow
Copy link
Contributor

@pelszkow pelszkow commented Dec 9, 2020 •
edited
Loading

Ticket: 40740

@pelszkow pelszkow requested review from jdanieck and sdurawa December 9, 2020 14:43
@pelszkow pelszkow force-pushed the constant_attr_to_visitor branch 6 times, most recently from 6f35598 to 81f4850 Compare December 15, 2020 06:48
@pelszkow pelszkow force-pushed the constant_attr_to_visitor branch from 81f4850 to 22a2cf9 Compare December 15, 2020 10:19
@pelszkow pelszkow marked this pull request as ready for review December 15, 2020 10:19
@pelszkow pelszkow requested review from a team December 15, 2020 10:19
jdanieck
jdanieck suggested changes Dec 15, 2020
View reviewed changes
ngraph/core/include/ngraph/op/constant.hpp Show resolved Hide resolved
inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp Outdated Show resolved Hide resolved
inference-engine/tests/functional/inference_engine/ir_serialization/serialize.cpp Outdated Show resolved Hide resolved
inference-engine/tests/functional/inference_engine/ir_serialization/serialize.cpp Outdated Show resolved Hide resolved
inference-engine/tests/functional/inference_engine/ir_serialization/serialize.cpp Outdated Show resolved Hide resolved
inference-engine/tests/functional/inference_engine/ir_serialization/serialize.cpp Outdated Show resolved Hide resolved
inference-engine/src/transformations/src/transformations/serialize.cpp Outdated Show resolved Hide resolved
inference-engine/src/transformations/src/transformations/serialize.cpp Outdated Show resolved Hide resolved
inference-engine/src/transformations/src/transformations/serialize.cpp Show resolved Hide resolved
inference-engine/src/transformations/src/transformations/serialize.cpp Outdated Show resolved Hide resolved
@pelszkow pelszkow requested review from sdurawa and jdanieck December 16, 2020 12:55
@pelszkow pelszkow requested a review from iefode December 16, 2020 14:03
@ilyachur ilyachur mentioned this pull request Dec 21, 2020
Closed
jdanieck
jdanieck reviewed Dec 22, 2020
View reviewed changes
inference-engine/src/transformations/src/transformations/serialize.cpp Outdated

// Return false because we didn't change nGraph Function
return false;
}

namespace {
std::string bestBinPath(const std::string &xmlPath, const std::string &binPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pelszkow please explain why you need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because some e2e tests are failing. Some test pass empty path here.

openvino/inference-engine/include/cpp/ie_cnn_network.h

Line 243 in e9b89b0

void serialize(const std::string& xmlPath, const std::string& binPath = "") const {

Copy link
Contributor

Choose a reason for hiding this comment

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

@ilyachur what is expected behaviour of InferenceEngine::CNNNetwork::serialize() when binPath is empty? It's not really specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose that we need to create the bin file with the same name as xml file if bin path is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. So current approach is OK - .xml is replaced with .bin.

There is one issue though, we don't check that xml path is valid, and malicious user can pass shorter than 3 character string and we will have out of bounds access.

I would do what follows:
if (xml_path is not valid path (non-empty + alphanumeric characters only?) )
throw
else if (xml_path ends with .xml)
bin_path = xml_path with replaced .xml with .bin
else
bin_name = xml_path + ".bin"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have the next logic:

if (bin_path is empty) {
    if (xml_path is not valid path (non-empty + alphanumeric characters only?) )
        throw
    else if (xml_path ends with .xml)
        bin_path = xml_path with replaced .xml with .bin
    else
        // Looks like if xml_path doesn't have .xml as an extension it is not valid path
        throw
}

Copy link
Contributor Author

@pelszkow pelszkow Dec 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have just realize that we are discuss about filename validation and not about whole path (with filename). / or c:\ are not alnum only.
When I add this validation I get error:

[ ERROR ] Can't get executable graph: Check 'std::all_of(begin(path), end(path), [](char c) { return std::isalnum(c); })' failed at ../inference-engine/src/transformations/src/transformations/serialize.cpp:517:
Filename for xml file contains invalid characters (alnum valid only): "/mnt/data/workspace/openvino/openvino/build/install/deployment_tools/exec_graph.xml"

Copy link
Contributor

Choose a reason for hiding this comment

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

@pelszkow Did you fix the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I drop isalnum path validation idea. Without this validation (is not present in master) all works fine.

iefode
iefode approved these changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@iefode iefode left a comment

Choose a reason for hiding this comment

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

IE test part LGTM

jdanieck
jdanieck suggested changes Dec 22, 2020
View reviewed changes
inference-engine/src/transformations/src/transformations/serialize.cpp Outdated

// Return false because we didn't change nGraph Function
return false;
}

namespace {
std::string bestBinPath(const std::string &xmlPath, const std::string &binPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. So current approach is OK - .xml is replaced with .bin.

There is one issue though, we don't check that xml path is valid, and malicious user can pass shorter than 3 character string and we will have out of bounds access.

I would do what follows:
if (xml_path is not valid path (non-empty + alphanumeric characters only?) )
throw
else if (xml_path ends with .xml)
bin_path = xml_path with replaced .xml with .bin
else
bin_name = xml_path + ".bin"

@ilyachur ilyachur merged commit 2bc18c2 into openvinotoolkit:master Dec 23, 2020
@pelszkow pelszkow deleted the constant_attr_to_visitor branch January 11, 2021 06:35
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Jan 14, 2021
@pelszkow @mryzhov
Constant attr to visitor (openvinotoolkit#3540)
5ac0704 
* Add new path for constant in IR serializer.

* Apply suggestion from review.

* Unique name for temporary test file

* Switch from static to constant member function - GetTestName

* Ensure bin path is not empty.

* Compare Constants op by string values converted to float.

* Add path validation.

Co-authored-by: Patryk Elszkowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdanieck jdanieck jdanieck approved these changes

@iefode iefode iefode approved these changes

@ilyachur ilyachur ilyachur approved these changes

@GlebKazantaev GlebKazantaev Awaiting requested review from GlebKazantaev

@sdurawa sdurawa Awaiting requested review from sdurawa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pelszkow @ilyachur @iefode @jdanieck @sdurawa