Fix divide-by-zero when page buf page size is 0

[derobins]

If a corrupt file sets the page buffer size in the superblock to zero, the library could attempt to divide by zero when allocating space in the file. The library now checks for valid page buffer sizes when reading the superblock message.

Fixes oss-fuzz issue 58762

[fortnern]

Could this be an assertion, since we already checked it when reading the file?

[derobins]

I prefer to not use asserts for error checking, aside from verifying input parameters, since a bunch of the CVE fixes have involved switching an assert out for real error checking (as here, in H5Fsuper.c). What if there were an overflow and something scribbled 0s on the struct member?

[fortnern]

It's not practical to protect against those types of memory errors by checking for their effects. I feel strongly that asserts should be used for these sorts of internal consistency checks, otherwise we'd have to use error checks everywhere at a substantial performance penalty

[fortnern]

Implementing a policy that internal consistency checks should be error checks and not asserts also creates an incentive to not add internal consistency checks.

[jhendersonHDF]

Agreed with @fortnern's points and I'd make the argument that asserts are exactly the appropriate tool to use when checking for something that may have overflowed. That's an internal consistency check for something that should not happen. Throwing an error in that case is nothing more than confusing.

[derobins]

I can switch it to an assert, but I'm a fan of "test what you ship" vs. "debug builds have the error checks". Also, our intuition for "should not happen" isn't always great, as we've had to switch asserts to real error checking to fix multiple issues, some of which were CVEs (again, as in this PR).

I also seriously doubt a couple of extra if statements are going to cause significant overhead. The vast majority of non-trivial statements in our library involve a bonus if check.

[qkoziol]

Generally speaking, I'm trending toward: "use errors for things that happen outside your code (file not found, bad user argument, out of memory, etc) and use asserts for things you do to yourself inside your code (weird, that shouldn't happen!)"

If you are confused about the state of your code, it's probably an assertion issue and you shouldn't really trust throwing an error - there's nothing the user can do about your code being wrong. Errors should be for things that users can do something about.

Unfortunately, we need to treat "the file got corrupted" as an error (done to us), where we previously (naively) thought it was an assertion issue (done to ourselves).