Replaced last sprintf with snprintf

[seanm]

To replace the only remaining sprintf with safer snprintf the size of the buffer needs to be known.

But in this case will require non-trivial changes that need thought/discussion.

This is just to show the issue...

[seanm]

@byrnHDF @jhendersonHDF @hyoklee this last one is non-trivial... thoughts?

[jhendersonHDF]

This approach seems reasonable and in line with other changes we made to H5O code when trying to clear up buffer overflow issues due to not knowing the size of the message buffers. Of course if you update the H5O_msg_class_t structure there will be several other encode callbacks that need to be updated, but it doesn't seem like a particularly bad idea to me and will be trivial unless you want to add new checks in all the encode routines to make sure buffers aren't overflowed. https://github.com/HDFGroup/hdf5/blob/develop/src/H5Omessage.c#L1586 and https://github.com/HDFGroup/hdf5/blob/develop/src/H5Omessage.c#L1975-L1976 are where the encode callback gets made and, at least at first glance, mesg->raw_size should contain the size of the allocated buffer for the message in the latter case. You'd need to calculate the remaining size in the buffer based on the current position in the message buffer of the uint8_t *p; variable in that code, but it seems perfectly doable.

For the former case, it looks like that function is currently called in 7 places and each of those places appear to have some calculated size that could be used, though in several places you will also have to make adjustments to the size value passed based on a pointer into the message buffer. In short, seems reasonable but will definitely be a decent bit of work to have a reasonable value to pass for p_size in every case.

[seanm]

This approach seems reasonable and in line with other changes we made to H5O code when trying to clear up buffer overflow issues due to not knowing the size of the message buffers.

Is this changing public API? Any need for some kind of API version bump or anything like that?

...unless you want to add new checks in all the encode routines to make sure buffers aren't overflowed.

I don't want to do that now. But it's probably a good idea, and will become possible with this groundwork.

My motivation now is just to fix the compiler warnings about sprintf.

[jhendersonHDF]

Is this changing public API? Any need for some kind of API version bump or anything like that?

It doesn't look like any sort of public API changes are needed here. It looks to all be in private code.

I don't want to do that now. But it's probably a good idea, and will become possible with this groundwork.

As sort of a half-measure, you could consider passing SIZE_MAX for the other places with the new p_size parameter, similar to what we do in https://github.com/HDFGroup/hdf5/blob/develop/src/H5Odtype.c#L1328-L1333, to signal that the buffer size checks should be skipped. Not ideal, but it's a quick solution and doesn't really change the library's current status in regards to checking buffer sizes on encode. Then you could just pass the correct size for p_size to H5O__mtime_encode.

[seanm]

It doesn't look like any sort of public API changes are needed here. It looks to all be in private code.

Oh ok great! I thought this was more of a breaking change.

As sort of a half-measure, you could consider passing SIZE_MAX for the other places with the new p_size parameter

Yeah, where I can't trivially determine the size, I'll do that. As you say, it's no worse than today. :)

[seanm]

So, I had to use SIZE_MAX in 3 of 4 call sites. :( There's a cascading effect now, where those call sites themselves need their function signature changed so their callers pass the buffer length.

But that can be some future PR...

[qkoziol]

LGTM

[seanm]

@qkoziol thanks. I've updated the commit message and removed the Draft flag.