Fix GH-15628: php_stream_memory_get_buffer() not zero-terminated

[cmb69]

We're reasonably sure that appending the NUL is not an OOB write, since the memory stream implementation uses zend_string APIs instead of fiddling with the buffer.

We don't add a regression test because that would require to set up something in the zend_test extension, and regressions are supposed to be caught by external consumers of this API, such as mailparse.

[cmb69]

Sorry, fix is borked; back to draft.

[cmb69]

@nielsdos, this requires an additional check for interned strings, and the assumption that these are always zero-terminated. I don't like that hackery, but it might be okay.

[nielsdos]

This should probably be fixed in the code where we modify the string, instead of the place where we get the buffer.

[cmb69]

Apparently, there are only two places where we would need to append a trailing NUL (less than I thought). And with this PR, the mailparse issues are gone.

[nielsdos]

Perhaps setting the NUL terminator can be moved to the place where the reallocation code is called, but this also works. LGTM

[cmb69]

Perhaps setting the NUL terminator can be moved to the place where the reallocation code is called, […]

Supposedly slightly more efficient, but I find doing it after memcpy()/memset() is slightly more intuitive. Let's see what @bukka thinks about this.

[bukka]

I would prefer to have zend_string API - maybe just some inline function that does memset sets '\0' at the end. I guess we might find more use case for it and seems cleaner to me.

[cmb69]

I would prefer to have zend_string API - maybe just some inline function that does memset sets '\0' at the end.

I'm not quite sure what you mean; just something like

static zend_always_inline void zend_string_terminate(zend_string *s)
{
	ZSTR_VAL(s)[ZSTR_LEN(s)] = '\0';
}

I'm not against it, but I don't think we should introduce a new API in a stable branch (note that this targets PHP-8.2).

[cmb69]

This could be simplified

Suggested change

	memset(ZSTR_VAL(ms->data) + old_size, 0, newsize - old_size);
	ZSTR_VAL(ms->data)[ZSTR_LEN(ms->data)] = '\0';
	memset(ZSTR_VAL(ms->data) + old_size, 0, newsize - old_size + 1); // +1 to zero-terminate

[bukka]

I was initially thinking about having wrappers around memset and memcpy but that doesn't really make much sense. The zend_string_terminate could be an option or maybe we could introduce alternative to zend_string_realloc that would do the termination - e.g. zend_string_realloc_terminated.

I guess it would be probably better to add in master though. I'm fine with this going as a bug fix in the meantime.