Added sanity checks before allocating memory after reads

[0sidharth]

Aims to fix #3606 . Went through the 273 instances and added sanity checks. Many of these instances already had sanity checks and all of them vary a lot, so I have adapted what I thought would be optimal for the ones that did not have any checks. It's my first PR in PCL, so please point out any miscellaneous mistakes too.

[kunaltyagi]

Thanks for the PR @0sidharth

This is just a partial review, since I feel a lot of the code needs minor tweaking.

EDIT: Do check the errors on the CI

[larshg]

I looked quickly at the style guide, but there are no mentioning of how sanity checks should generally be implemented.

but I would suggest in stead of the following:

///////////////////////////////////////////////////////////////////////////////////////////////////////////////////
template<typename real> void
pcl::BivariatePolynomialT<real>::readBinary (std::istream& os)
{
  memoryCleanUp ();
  os.read (reinterpret_cast<char*> (&this->degree), sizeof (int));
  unsigned int paramCnt = getNoOfParametersFromDegree (this->degree);
  parameters = new real[paramCnt];
  os.read (reinterpret_cast<char*> (&(*this->parameters)), paramCnt * sizeof (real));
  if (os.bad())
  {
    PCL_THROW_EXCEPTION (pcl::IOException, "Failure in reading degree from file");
  }
  else if (os.fail())
  {
    PCL_THROW_EXCEPTION (pcl::IOException, "failbit set while reading degree (formatting or extraction error");
  }
  else
  {
    unsigned int paramCnt = getNoOfParametersFromDegree (this->degree);
    parameters = new real[paramCnt];
    os.read (reinterpret_cast<char*> (&(*this->parameters)), paramCnt * sizeof (real));
  }
}

write it like this:

///////////////////////////////////////////////////////////////////////////////////////////////////////////////////
template<typename real> void
pcl::BivariatePolynomialT<real>::readBinary (std::istream& os)
{
  memoryCleanUp ();
  os.read (reinterpret_cast<char*> (&this->degree), sizeof (int));
  
  // Missing a check here on degree?
 
  unsigned int paramCnt = getNoOfParametersFromDegree (this->degree);
  parameters = new real[paramCnt];
  os.read (reinterpret_cast<char*> (&(*this->parameters)), paramCnt * sizeof (real));
  
  if (os.bad())
  {
    PCL_THROW_EXCEPTION (pcl::IOException, "Failure in reading degree from file");
  }
  
  if (os.fail())
  {
    PCL_THROW_EXCEPTION (pcl::IOException, "failbit set while reading degree (formatting or extraction error");
  }

  unsigned int paramCnt = getNoOfParametersFromDegree (this->degree);
  parameters = new real[paramCnt];
  os.read (reinterpret_cast<char*> (&(*this->parameters)), paramCnt * sizeof (real));
}

I'm sure if there are any advantages writing two ifs after each other over, if-else - but I guess the two errors are not necessarily dependent on each other or related, so I think it gives better clarity that they are seperated. It is ofc. the same output input stream that are checked. Got fooled by the parameter name (os).

Genereally I don't think we need the last else clauses. If one of the errors are true, an exception is thrown. If not, we can continue execution the following code.

but in the end its a "style" and I just skimmed it quickly, but generally I think it looks good.
But wait for others to chime in before going ahead and changing the code. I'm not a maintainer, just wanted to give my thoughts.

[larshg]

@kunaltyagi would it make sense to check (not sure what can be checked for though) the istream object in top of the method - before reading anything?

[larshg]

Just quickly looked istream - maybe use good instead? EDIT: It also checks if EOF was reached, so not sure if it would be set, if it is a last variable that is read or it is only set if you try to read beyond the file.

Edit2: Its also possible to use ! - which is synonym for fail, which checks both fail and bad bit.

Depends if we want detailed description if it was either bad or fail - whatever the differences can be here.

[0sidharth]

@larshg I had considered good(), however I was a bit confused as while going through all the files that have an instance of read, they have largely varying sanity checks, varying in what they are checking and how they are indicating a fault, so I just went with what felt good to me. Also I am checking badbit before failbit so I dont think two ifs would make much of a difference, wouldn't mind making the changes though if a maintainer feels they are better.

[0sidharth]

@kunaltyagi I looked over the CI errors, but couldn't find errors related to my code or specific lines where formatting fails. As for build, it is building on my Ubuntu 16.04, I don't understand why it is failing on 19.10 from the error message. Could you or any other maintainers guide me on what to do regarding these?

[larshg]

@0sidharth Okay :) Well, its nice to get it more unified then!
Might be a good idea to add it to the style section as well, whatever solution get chosen.

[kunaltyagi]

For the CI:

• Run the format target to get format CI working
• Ignore the 19.10 for a little while
• I've rerun Windows CI, maybe the test failure was a fluke, maybe some one merged something wrong on master

use good instead

Using good wouldn't be a way forward. It checks if the next read will succeed. Using ! is a possibility. For reads, bad can only happen for first read, if it succeeds, all others will fail(). So maybe we can use bad after the first read and ! for the rest. This is for read only, and not write

would it make sense to check ... the istream object ... before reading anything?

The incoming stream is assumed to be working, just like elsewhere in PCL (eg: ostream<< functions), so no need to check at the top of the function. Though it is good defensive-coding strategy, it doesn't fit in with existing code.

[taketwo]

if (os.bad())
{
  PCL_THROW_EXCEPTION (pcl::IOException, "Failure in reading degree from file");
}
  
if (os.fail())
{
  PCL_THROW_EXCEPTION (pcl::IOException, "failbit set while reading degree (formatting or extraction error");
}

If we are going to repeat this in many places, it's maybe worth adding a specialized macro. Something like PCL_CHECK_IO_STREAM, that checks both bad and fail, and bails out with IOException with an appropriate message.

PCL_CHECK_IO_STREAM(os, "degree")

[0sidharth]

@taketwo Currently I have implemented a function to do the same, should I change it to a macro?

[SergioRAgostinho]

Hmm. I think I do prefer a function rather than a macro. But it don't think it needs to be a static function of IOException.

[taketwo]

PCL_THROW_EXCEPTION automatically includes file name, code line, and function name in the exception. This information is helpful for debugging. If we move these throws inside a helper function, as you proposed, then this information will not reflect the actual calling site, but rather the helper function.

I know, macros are "bad", but I think in this case they are justified. I'm absolutely open to other suggestions, though.

[SergioRAgostinho]

If we move these throws inside a helper function, as you proposed, then this information will not reflect the actual calling site, but rather the helper function.

In light of this I yield my original position in favor of the macro.

[0sidharth]

Alright, I have changed it from a function to a macro, please review it.

[kunaltyagi]

Partially reviewed, but some points mentioned are present in multiple of the files touched.

[SergioRAgostinho]

@0sidharth don't force push until the review is over. If you force push GitHub is no longer able to show reviewers only the subset of changes which were added and we need to check everything again. Not very time efficient.

You should only force push after the reviews are complete, if we ask you to squash commits or rebase with master.

[kunaltyagi]

Changeset looks good, structurally. Only a few topical issues need to be addressed.

[kunaltyagi]

LGTM. Waiting on sergio

[0sidharth]

@SergioRAgostinho

[SergioRAgostinho]

Other stuff to have a look first. You'll need to wait.

[SergioRAgostinho]

Overall LGTM. We just need to address this new type of Exception which I believe to be necessary when some of the parsed values don't make sense.

[SergioRAgostinho]

This exception is complaining about a bad/incorrect numerical value. It's not really an IO issue anymore. This is the wrong type of exception to throw in this situation, but unfortunately I don't see an existing PCL Exception which matches what we would likely want to express here. Maybe BadArgumentException? Otherwise I'm open to create a new exception type and I'm looking for suggestions on a name which better expresses what is happening here.

[0sidharth]

I don't think BadArgumentException is suited for this situation. Perhaps a new exception BadValueInFileException?

[SergioRAgostinho]

Maybe just BadValue so that it can be used in other contexts.

[kunaltyagi]

Should we add multiple modules in the label for PR like this?

[SergioRAgostinho]

I believe the usual policy is to not add anything if it touches a lot of modules. If they're low in number feel free to.

[kunaltyagi]

Use auto& or const auto& depending on context (L120, L165). There's no need for making extra copies.

[kunaltyagi]

Pinging for updates