Check if read succedded before reading again/allocating memory

[kunaltyagi]

Your Environment

• PCL Version: HEAD

Context

Bad parsing is inevitable, specially if operations are performed on read data without validation

Expected Behavior

Graceful error detection

Current Behavior

Files: 273 instances. One sample provided:

• features/src/narf.cpp:576...
• More here

Possible Solution

Sanity checks on input stream after reading before allocating memory

[omar-bakr]

Should i fix the 273 instances ?

[shrijitsingh99]

@kunaltyagi Could you expand upon what exactly you mean by "graceful error detection" as failure to read the stream can lead to unexpected behavior.
Not sure what we be a good method to handle this.

[kunaltyagi]

failure to read the stream can lead to unexpected behavior

This should be factored in the operation. Failure to read should inform the user with proper debug information programmatically (like return a false, or raise exception or something) along with log in debug mode.

[SergioRAgostinho]

Raise exception and let the upper layers in the stack decide what to do with it.

[shrijitsingh99]

Since there exists no exception for failure to read, @SergioRAgostinho should I go ahead and create a new exception in pcl/exceptions.h as it would be useful at multiple places?

[SergioRAgostinho]

What about this one

pcl/common/include/pcl/exceptions.h

Lines 176 to 188 in e5b8fe5

	/** \class IOException
	* \brief An exception that is thrown during an IO error (typical read/write errors)
	*/
	class IOException : public PCLException
	{
	public:
	IOException (const std::string& error_description,
	const char* file_name = nullptr,
	const char* function_name = nullptr,
	unsigned line_number = 0)
	: pcl::PCLException (error_description, file_name, function_name, line_number) { }
	} ;

Do you feel it's too generic?

[shrijitsingh99]

Yeah I thought it would be too generic but after looking at all the case, I guess most cases would fit under IOException
If a need for a more specific error is needed will raise it over here, for now, will use IOException.

[0sidharth]

try {
file.read(reinterpret_cast<char*>(&surface_patch_pixel_size_), sizeof(surface_patch_pixel_size_));
surface_patch_ = new float[surface_patch_pixel_size_*surface_patch_pixel_size_];
}
catch(pcl::IOException& e)
{
std::cerr << e.what() << std::endl;
}

Is something like this alright? Just want to confirm before actually implementing it to the 273 instances.

Would it perhaps be better to write a function that performs this sanity check, instead of writing try and catch statements at all the instances?

[SergioRAgostinho]

In your example file is of std::istream type which knows nothing about pcl's exceptions and by default it does not throw exceptions. It uses a status bit to indicate if something went wrong.
If such case happens, then we want to throw a pcl::IOException.

Answering directly your question: the snippet is not what we want. Take some time to read https://en.cppreference.com/w/cpp/io/basic_istream/read carefully.

[0sidharth]

try
{
file.read(reinterpret_cast<char*>(&surface_patch_pixel_size_), ``sizeof(surface_patch_pixel_size_));
surface_patch_ = new float[surface_patch_pixel_size_*surface_patch_pixel_size_];
if (file.std::ios::bad())
{
PCL_THROW_EXCEPTION (pcl::IOException, "Failure reading data from file");
}
else if(file .std::ios::fail())
{
PCL_THROW_EXCEPTION (pcl::IOException, "Failure in opening file");
}
}
catch(pcl::IOException& e)
{
std::cerr << e.what() << std::endl;
}

What about this?

[kunaltyagi]

There's no value in catching an exception you just threw. This exception handling (try...catch) is for the person who called the function

[0sidharth]

Oh, right. So this snippet without the try and catch, and checking for error before allocating surface_patch_ looks okay?

[kunaltyagi]

No @0sidharth , it is syntactically wrong. Please try to compile your snippets. I'd recommend a Unix machine or Windows + MS VS 2019 or https://godbolt.org as good places to start.

[0sidharth]

  file.read(reinterpret_cast<char*>(&surface_patch_pixel_size_), sizeof(surface_patch_pixel_size_));
  if (file.std::ios::bad())
  {
    PCL_THROW_EXCEPTION (pcl::IOException, "Failure reading data from file");
  }
  else if(file .std::ios::fail())
  {
    PCL_THROW_EXCEPTION (pcl::IOException, "Failure in opening file");
  }
  else
  {
    surface_patch_ = new float[surface_patch_pixel_size_*surface_patch_pixel_size_];
  }

I was talking about this @kunaltyagi , and it is compiling for me so I cannot figure out which syntatic error you are referring to?

[kunaltyagi]

The snippet provided here is correct. I've 2 more comments on this snippet:

• I'd use file.fail() or file.bad() instead of qualifying the function
• Please also note that failbit can be set for reasons other than "failure to open", including failure in the call file.read()

[stale]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.