On Mon, Aug 16, 2021 at 6:05 PM Henri Verbeet hverbeet@gmail.com wrote:
On Fri, 13 Aug 2021 at 16:15, Matteo Bruni mbruni@codeweavers.com wrote:
static bool vkd3d_string_buffer_resize(struct vkd3d_string_buffer *buffer, int rc) {
- unsigned int new_buffer_size = buffer->buffer_size * 2;
- unsigned int new_buffer_size = max(buffer->buffer_size * 2, 32); char *new_buffer;
- new_buffer_size = max(new_buffer_size, 32);
- if (rc < buffer->buffer_size - buffer->content_size)
return true;That doesn't quite handle the case where "rc" is -1. Perhaps it's fine to assume it isn't these days, but in that case we might as well get rid of the rest of the code handling that case as well. The patch also seems a little superfluous though; we already have a similar check in vkd3d_string_buffer_vprintf().
I entirely forgot about the rc == -1 case... I can fix that.
Otherwise, I know that I only noticed this issue and started writing these patches because I was in fact getting out of memory errors :) IIRC vkd3d_string_buffer_resize() would at least double the size of the buffer every time it's called and vkd3d_string_buffer_vprintf() in practice ended up writing 0 bytes to the buffer relatively often (I haven't investigated that part, but it seems legit, if weird). Also apparently the code often doubled the size one too many times. I'll check that again but I don't think I saw that after this patch.
We would do an unnecessary resize when calling vkd3d_string_buffer_vprintf() the first time after a vkd3d_string_buffer_clear(), although the best way to fix that would seem to be simply getting rid of the vkd3d_string_buffer_resize() call responsible for that, like patch 2/3 in this series does. It was added by commit 5fb9bcdd148ed728d90a420f209cdfcc03af24ac, but that same commit also adds the max() in vkd3d_string_buffer_resize() that should make it superfluous.
I guess one possible mitigation would be to rename vkd3d_string_buffer_resize() to vkd3d_string_buffer_reserve() or similar, to clarify that it won't reallocate the buffer when unnecessary.