tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
330 stars 97 forks source link

Fix Heap-buffer-overflow WRITE in H5MM_memcpy #210

Closed sashashura closed 10 months ago

sashashura commented 11 months ago

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58262

The root cause is that H5Sget_simple_extent_dims doesn't fail and doesn't initialize *nfields for scalar types, so it is left zero. Behavior of calloc call with zero size is implementation dependent, but in this case it returns a non null memory which is valid to use only with free.

sashashura commented 11 months ago

I do not get the fix, actually.

Inside of H5A__read it calls H5MM_memcpy and tries to copy 16 bytes to fieldnames_vl which is allocated with calloc(0, 16). The calloc documentation doesn't say what is the return value if the first argument (number of objects) is zero, but says: If size is zero, the behavior is implementation defined (null pointer may be returned, or some non-null pointer may be returned that may not be used to access storage). All in all, when calloc is asked to allocate zero memory it doesn't fail with null, but returns a pointer which can be used for realloc or free, but which is illegal to dereference. The first argument for calloc *nfields is zero, because H5Sget_simple_extent_dims didn't fail (err >= 0), but also didn't set a value to *nfields. It doesn't look as a bug in HDF5, because it is documented in H5S_get_simple_extent_dims (called internally) that This function may not be meaningful for all types of dataspaces. Return: Success: Number of dimensions. Zero implies scalar. It simply returns in scalar case and doesn't touch dims.

You know better if this is invalid situation and should be rejected at all, but it looks to me, that it is ok to set the number of objects to 1 in case of scalar.

tbeu commented 11 months ago

Need to get the CI working first which is broken since merge of #198.

tbeu commented 11 months ago

Travis CI reports Error "https://github.com/modelica-tools/msl-release" for certain test cases.

sashashura commented 11 months ago

Do you mean the pull request broke some tests?

tbeu commented 11 months ago

Do you mean the pull request broke some tests?

Yes, this seems to be the case.

tbeu commented 10 months ago

I fixed the CI and rebased your PR branch such that you can now see the crashes of the testsuite.

sashashura commented 10 months ago

My bad, I have switched to using err last minute and didn't notice it is overridden few lines above.

sashashura commented 10 months ago

Travis didn't start for some reason.

tbeu commented 10 months ago

Travis CI credit balance is negative again.

sashashura commented 10 months ago

But it ran for https://github.com/tbeu/matio/pull/214 which was made a bit later. May I ask why you use two build systems - appveyor and travis, but not github actions at the same time?

sashashura commented 10 months ago

Looks like everything has passed except 1 of 3 macos builds that timed out.

sashashura commented 10 months ago

The https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58262 was closed!