Add H5Tdecode2, rename and deprecate H5Tdecode #5213
Conversation
mattjala
added
Merge - Develop Only
mattjala
requested review from
jhendersonHDF,
byrnHDF,
derobins,
brtnfld,
epourmal,
lrknox,
fortnern,
qkoziol,
vchoi-hdfgroup,
bmribler and
glennsong09
as code owners
test/dtypes.c
Outdated
There was a problem hiding this comment.
There should be a new test that exercises the deprecated H5Tdecode1 routine.
| // Call C function to decode the binary object description | ||
| hid_t encoded_dtype_id = H5Tdecode(encoded_buf); | ||
| hid_t encoded_dtype_id = H5Tdecode2(encoded_buf, tmp_buf_size); | ||
|
|
There was a problem hiding this comment.
Thank you for addressing the issue, but I'm sorry, I made a mistake in the original comment. If encoded_buf is not NULL then buf_size will already be the size of the encoded buffer. You can safely use buf_size in H5Tdecode2 as you originally did. I'm sorry again!
There was a problem hiding this comment.
Without having to call H5Tencode to get the size, that is. Checking buf_size is a good idea though. However, if you want to put a fix in DataType::close() to set encoded_buf to NULL then checking for NULL encoded_buf alone will be good enough. Question me if you see anything I said questionable. :D
| @@ -13883,7 +13886,7 @@ public static long H5Tdecode(byte[] buf) throws HDF5LibraryException, NullPointe | |||
| return id; | |||
| } | |||
|
|
|||
| private synchronized static native long _H5Tdecode(byte[] buf) | |||
There was a problem hiding this comment.
At first I was going to mention that there should be the corresponding JNI changes, but it seems there is no _H5Tdecode implementation currently. @byrnHDF We should either implement that or remove this function from being available.
src/H5T.c
Outdated
| @@ -3812,6 +3807,9 @@ H5T_decode(size_t buf_size, const unsigned char *buf) | |||
| if (NULL == (f = H5F_fake_alloc((uint8_t)0))) | |||
| HGOTO_ERROR(H5E_DATATYPE, H5E_CANTALLOC, NULL, "can't allocate fake file struct"); | |||
|
|
|||
| if (buf_size < 2) | |||
There was a problem hiding this comment.
Not a big fan of an explicit comparison against the value 2 here. Using 2 occurrences of the H5_IS_BUFFER_OVERFLOW with a size of 1, for example, might be more verbose, but IMO is a lot clearer about what's going on.
When provided malformed or too-small buffers, H5Tdecode() could crash due to walking off the end of the buffer. The new buffer size parameter allows this to be reliably avoided.
Resolves #4661