-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Resolve SIGSEGV error in image_data_layer.cpp #4218
Conversation
@@ -46,6 +46,9 @@ void ImageDataLayer<Dtype>::DataLayerSetUp(const vector<Blob<Dtype>*>& bottom, | |||
lines_.push_back(std::make_pair(line.substr(0, pos), label)); | |||
} | |||
|
|||
// Check that image list is not empty. | |||
CHECK_GT(lines_.size(), 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.
You should use method empty()
instead.
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.
Hey felix, how is that better? I think the functionality is same.
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.
It's better practice, more idiomatic.
It doesn't matter for a vector, but for a list it does, in C++98, the complexity of list.size()
is not guaranteed to be constant.
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.
Okay. Thanks. Should I make the change and submit another PR?
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.
Just edit this PR. You need to git commit --amend
your patch and then push force to your branch and it will automatically update this PR.
Edited the code as per @flx42's suggestions. |
@@ -46,6 +46,9 @@ void ImageDataLayer<Dtype>::DataLayerSetUp(const vector<Blob<Dtype>*>& bottom, | |||
lines_.push_back(std::make_pair(line.substr(0, pos), label)); | |||
} | |||
|
|||
// Check that image list is not 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.
You can omit this comment -- it does not add anything to the code.
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.
Sure. Should I add a terminal output similar to line 73?
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.
That would be a little nicer, yes.
Thanks, this looks good except one comment comment and then please squash (and force push) so your history appears as a single commit for this simple change (and perhaps use |
Edited the code as per @longjon's suggestions. |
Looks good, thanks! |
Thanks @malreddysid |
Hi @malreddysid As part of #4203 I thought I would add a quick unit test for this PR and found the following:
(the Once #4203 is merged, could you look at adding some tests for this case? For reference my two testcases are:
|
I don't know if it's as simple as that. If one has the following test:
This also counts as an empty file, but simply checking pos will not be enough, as there are multiple spaces. Note that this does not result in a segmentation violation, but does give a bit of a cryptic error message. |
#1971 could take care of all such bugs. But until that is implemented, I guess we could look for file extensions (.jpg, .png, etc) that are supported by OpenCV, along with 'pos' value and length of label substring to check the validity of the file. |
Ok, if #1971 addressed this, then I will move the comments and tests there instead. Thanks! |
Resolve SIGSEGV error in image_data_layer.cpp
PR to resolve issue #4186.
Added check in image_data_layer.cpp to ensure that if an empty list is given as source to ImageData layer, segmentation fault won't occur.