Resolve SIGSEGV error in image_data_layer.cpp

[malreddysid]

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.

[flx42]

You should use method empty() instead.

[malreddysid]

Hey felix, how is that better? I think the functionality is same.

[flx42]

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.

[malreddysid]

Okay. Thanks. Should I make the change and submit another PR?

[flx42]

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.

[malreddysid]

Edited the code as per @flx42's suggestions.

[longjon]

You can omit this comment -- it does not add anything to the code.

[malreddysid]

Sure. Should I add a terminal output similar to line 73?

[longjon]

That would be a little nicer, yes.

[longjon]

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 git commit --amend to write a slightly more descriptive one-line message, e.g., "check for non-empty ImageData filelist" (note that the information in the current message is already readily obtained with git log --stat)).

[malreddysid]

Edited the code as per @longjon's suggestions.

[longjon]

Looks good, thanks!

[ajtulloch]

Thanks @malreddysid

[elezar]

Hi @malreddysid

As part of #4203 I thought I would add a quick unit test for this PR and found the following:

• When the file is COMPLETELY empty then the code works as expected.
• When there is any whitespace in the file (e.g. a single newline), the following error is given:

[  DEATH   ] E0601 06:06:03.550549  5324 io.cpp:80] Could not open or find file 
[  DEATH   ] F0601 06:06:03.550745  5324 image_data_layer.cpp:77] Check failed: cv_img.data Could not load 
[  DEATH   ] *** Check failure stack trace: ***
[  DEATH   ]     @     0x7fc7170e7daa  (unknown)
[  DEATH   ]     @     0x7fc7170e7ce4  (unknown)
[  DEATH   ]     @     0x7fc7170e76e6  (unknown)
[  DEATH   ]     @     0x7fc7170ea687  (unknown)
[  DEATH   ]     @     0x7fc717ccedd5  caffe::ImageDataLayer<>::DataLayerSetUp()
[  DEATH   ]     @     0x7fc717cd8583  caffe::BasePrefetchingDataLayer<>::LayerSetUp()
[  DEATH   ]     @           0xad62ad  caffe::Layer<>::SetUp()
[  DEATH   ]     @           0xae04be  caffe::ImageDataLayerTest_WhitespaceFileRaiseError_Test<>::TestBody()
[  DEATH   ]     @           0xd4ed93  testing::internal::HandleExceptionsInMethodIfSupported<>()
[  DEATH   ]     @           0xd45817  testing::Test::Run()
[  DEATH   ]     @           0xd458be  testing::TestInfo::Run()
[  DEATH   ]     @           0xd459c5  testing::TestCase::Run()
[  DEATH   ]     @           0xd48718  testing::internal::UnitTestImpl::RunAllTests()
[  DEATH   ]     @           0xd489b7  testing::UnitTest::Run()
[  DEATH   ]     @           0x871b8f  main
[  DEATH   ]     @     0x7fc71179dec5  (unknown)
[  DEATH   ]     @           0x877d92  (unknown)
[  DEATH   ]     @              (nil)  (unknown)
[  DEATH   ] 

(the [ DEATH ] tags are from gtest).

Once #4203 is merged, could you look at adding some tests for this case?

For reference my two testcases are:

TYPED_TEST(ImageDataLayerTest, EmptyFileRaiseError) {
  typedef typename TypeParam::Dtype Dtype;

  // The following is needed to suppress the following warning:
  // [WARNING] ::Death tests use fork(), which is unsafe particularly in a
  //     threaded context.For this test, Google Test couldn't
  //     detect the number of threads.
  ::testing::FLAGS_gtest_death_test_style = "threadsafe";

  std::ofstream& output = this->stream();
  output.close();

  LayerParameter param;
  ImageDataParameter* image_data_param = param.mutable_image_data_param();
  image_data_param->set_source(this->filename_.c_str());
  ImageDataLayer<Dtype> layer(param);

  EXPECT_DEATH(layer.SetUp(this->blob_bottom_vec_, this->blob_top_vec_),
               "File is empty");

}

TYPED_TEST(ImageDataLayerTest, WhitespaceFileRaiseError) {
  typedef typename TypeParam::Dtype Dtype;

  // The following is needed to suppress the following warning:
  // [WARNING] ::Death tests use fork(), which is unsafe particularly in a
  //     threaded context.For this test, Google Test couldn't
  //     detect the number of threads.
  ::testing::FLAGS_gtest_death_test_style = "threadsafe";

  std::ofstream& output = this->stream();
  output << std::endl;
  output.close();

  LayerParameter param;
  ImageDataParameter* image_data_param = param.mutable_image_data_param();
  image_data_param->set_source(this->filename_.c_str());
  ImageDataLayer<Dtype> layer(param);

  EXPECT_DEATH(layer.SetUp(this->blob_bottom_vec_, this->blob_top_vec_),
               "File is empty");

}

[malreddysid]

Hi @elezar,

Thanks for the suggestion. After #4203 gets merged, I'll add a check on the 'pos' value to prevent empty line being pushed into 'lines'.

[elezar]

I don't know if it's as simple as that. If one has the following test:

std::ofstream& output = this->stream();
  // Insert 8 spaces into the test file.
  output << "        " << std::endl;
  output.close();

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.

[malreddysid]

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

[elezar]

Ok, if #1971 addressed this, then I will move the comments and tests there instead. Thanks!