php_stream_memory_get_buffer() not zero-terminated

Description

The documentation of zend_strings states:

It should be noted that while len stores the length without a trailing null byte, the actual string contents in val must always contain a trailing null byte.

According to this, php/pecl-mail-mailparse@3680710 has been commited a while ago. However, that caused an issue which had been reported as php/pecl-mail-mailparse#31.

The problem here is that php_stream_memory_get_buffer returns a zend_string which is not zero-terminated. Now, the question is whether that is a bug in php_stream_memory_get_buffer(), or whether zend_strings are always zero-terminated, unless they aren't.

A possible fix for php_stream_memory_get_buffer() might be:

 main/streams/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/main/streams/memory.c b/main/streams/memory.c
index f53084a6c3a..be8223fc72a 100644
--- a/main/streams/memory.c
+++ b/main/streams/memory.c
@@ -321,7 +321,7 @@ PHPAPI zend_string *_php_stream_memory_get_buffer(php_stream *stream STREAMS_DC)
 {
 	php_stream_memory_data *ms = (php_stream_memory_data*)stream->abstract;
 	ZEND_ASSERT(ms != NULL);
-	return ms->data;
+	return zend_string_init_fast(ZSTR_VAL(ms->data), ZSTR_LEN(ms->data));
 }
 /* }}} */

If this is not a bug in php_stream_memory_get_buffer(), an obvious fix for mailparse would be to revert php/pecl-mail-mailparse@3680710.

PHP Version

PHP 8.1

Operating System

any

Afaik zend_string should always be NUL terminated. It would be sad to have to init a new string though...

Well, I guess we could also do:

 main/streams/memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/main/streams/memory.c b/main/streams/memory.c
index f53084a6c3a..9e90a606d5c 100644
--- a/main/streams/memory.c
+++ b/main/streams/memory.c
@@ -321,6 +321,7 @@ PHPAPI zend_string *_php_stream_memory_get_buffer(php_stream *stream STREAMS_DC)
 {
 	php_stream_memory_data *ms = (php_stream_memory_data*)stream->abstract;
 	ZEND_ASSERT(ms != NULL);
+	ZSTR_VAL(ms->data)[ZSTR_LEN(ms->data)] = '\0';
 	return ms->data;
 }
 /* }}} */

But looking at the implementation of php_stream_memory_write() (which is called by php_stream_putc() among others), I suspect a lot of reallocation happening, so this new string initialization might not make that much of a difference. I wonder whether we should use a smart_str instead for the buffer.

I like your last patch better (didn't check whether ZSTR_LEN will go out of bounds though), but I believe it should be something along those lines.
To smart_str or not to smart_str is a bit orthogonal, it's hard to say without a benchmark.

See

php-src/Zend/zend_string.h

Lines 196 to 203 in 367f303

    
           static zend_always_inline zend_string *zend_string_init(const char *str, size_t len, bool persistent) 
        
           { 
        
           	zend_string *ret = zend_string_alloc(len, persistent); 
        
           	memcpy(ZSTR_VAL(ret), str, len); 
        
           	ZSTR_VAL(ret)[len] = '\0'; 
        
           	return ret; 
        
           }

zend_string_alloc() is supposed to allocate at least one byte more than requested. I'll make a PR shortly.

To smart_str or not to smart_str is a bit orthogonal, it's hard to say without a benchmark.

Right!

See (...) zend_string_alloc() is supposed to allocate at least one byte more than requested.

Well yes, I know this. But the reason why it is safe is here:

php-src/main/streams/memory.c

Line 58 in 367f303

ms->data = zend_string_realloc(ms->data, ms->fpos + count, 0);

because it uses the realloc of zend_string itself instead of something magical custom.

I'll make a PR shortly.

Great! Thanks.

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

Merge branch 'PHP-8.2' into PHP-8.3

Merge branch 'PHP-8.3'

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

Navigation Menu

Repository navigation

php_stream_memory_get_buffer() not zero-terminated #15628

Description

PHP Version

Operating System

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	static zend_always_inline zend_string zend_string_init(const char str, size_t len, bool persistent)
	{
	zend_string *ret = zend_string_alloc(len, persistent);

	memcpy(ZSTR_VAL(ret), str, len);
	ZSTR_VAL(ret)[len] = '\0';
	return ret;
	}

php_stream_memory_get_buffer() not zero-terminated #15628

Description

Description

PHP Version

Operating System

Activity

ndossche commented on Aug 29, 2024

cmb69 commented on Aug 29, 2024

ndossche commented on Aug 29, 2024

cmb69 commented on Aug 29, 2024

ndossche commented on Aug 29, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions