Skip to content

Minor improvements to ext/xml memory management #18071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 15, 2025
Merged

Minor improvements to ext/xml memory management #18071

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17f1931
Use zend_strings to avoid duplicating the tags
nielsdos Mar 14, 2025
5ccbd97
Remove pointless memset
nielsdos Mar 14, 2025
7166d54
Use zval_ptr_dtor_str where possible
nielsdos Mar 14, 2025
1a4f529
fix leak
nielsdos Mar 15, 2025
b90adb7
Fix windows
nielsdos Mar 15, 2025
93107b8
[ci skip] comments
nielsdos Mar 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 36 additions & 24 deletions ext/xml/xml.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ typedef struct {
int toffset;
int curtag;
zend_long ctag_index;
char **ltags;
zend_string **ltags;
bool lastwasopen;
bool skipwhite;
bool isparsing;
Expand Down Expand Up @@ -116,8 +116,6 @@ ZEND_GET_MODULE(xml)

#define XML_MAXLEVEL 255 /* XXX this should be dynamic */

#define SKIP_TAGSTART(str) ((str) + (parser->toffset > strlen(str) ? strlen(str) : parser->toffset))

static zend_class_entry *xml_parser_ce;
static zend_object_handlers xml_parser_object_handlers;

Expand All @@ -138,7 +136,7 @@ inline static unsigned short xml_encode_us_ascii(unsigned char);
inline static char xml_decode_us_ascii(unsigned short);
static void xml_xmlchar_zval(const XML_Char *, int, const XML_Char *, zval *);
static int xml_xmlcharlen(const XML_Char *);
static void xml_add_to_info(xml_parser *parser, const char *name);
static void xml_add_to_info(xml_parser *parser, zend_string *name);
inline static zend_string *xml_decode_tag(xml_parser *parser, const XML_Char *tag);

void xml_startElementHandler(void *, const XML_Char *, const XML_Char **);
Expand Down Expand Up @@ -324,7 +322,7 @@ static void xml_parser_free_ltags(xml_parser *parser)
if (parser->ltags) {
int inx;
for (inx = 0; ((inx < parser->level) && (inx < XML_MAXLEVEL)); inx++)
efree(parser->ltags[ inx ]);
zend_string_release_ex(parser->ltags[inx], false);
efree(parser->ltags);
}
}
Expand Down Expand Up @@ -553,7 +551,7 @@ static int xml_xmlcharlen(const XML_Char *s)
/* }}} */

/* {{{ xml_add_to_info() */
static void xml_add_to_info(xml_parser *parser, const char *name)
static void xml_add_to_info(xml_parser *parser, zend_string *name)
{
zval *element;

Expand All @@ -564,11 +562,10 @@ static void xml_add_to_info(xml_parser *parser, const char *name)
SEPARATE_ARRAY(Z_REFVAL(parser->info));
zend_array *arr = Z_ARRVAL_P(Z_REFVAL(parser->info));

size_t name_len = strlen(name);
if ((element = zend_hash_str_find(arr, name, name_len)) == NULL) {
if ((element = zend_hash_find(arr, name)) == NULL) {
zval values;
array_init(&values);
element = zend_hash_str_update(arr, name, name_len, &values);
element = zend_hash_update(arr, name, &values);
}

add_next_index_long(element, parser->curtag);
Expand All @@ -592,6 +589,17 @@ static zend_string *xml_decode_tag(xml_parser *parser, const XML_Char *tag)
}
/* }}} */

static zend_string *xml_stripped_tag(zend_string *tag_name, int offset)
{
if (offset == 0) {
return zend_string_copy(tag_name);
} else if (offset >= ZSTR_LEN(tag_name)) {
return ZSTR_EMPTY_ALLOC();
} else {
return zend_string_init(ZSTR_VAL(tag_name) + offset, ZSTR_LEN(tag_name) - offset, false);
}
}

static zval *xml_get_separated_data(xml_parser *parser)
{
if (EXPECTED(Z_TYPE_P(Z_REFVAL(parser->data)) == IS_ARRAY)) {
Expand Down Expand Up @@ -632,7 +640,7 @@ void xml_startElementHandler(void *userData, const XML_Char *name, const XML_Cha
if (ZEND_FCC_INITIALIZED(parser->startElementHandler)) {
zval args[3];
ZVAL_COPY(&args[0], &parser->index);
ZVAL_STRING(&args[1], SKIP_TAGSTART(ZSTR_VAL(tag_name)));
ZVAL_STR(&args[1], xml_stripped_tag(tag_name, parser->toffset));
array_init(&args[2]);

while (attributes && *attributes) {
Expand Down Expand Up @@ -663,15 +671,15 @@ void xml_startElementHandler(void *userData, const XML_Char *name, const XML_Cha
array_init(&tag);
array_init(&atr);

char *skipped_tag_name = SKIP_TAGSTART(ZSTR_VAL(tag_name));
zend_string *stripped_tag = xml_stripped_tag(tag_name, parser->toffset);
xml_add_to_info(parser, stripped_tag);

xml_add_to_info(parser, skipped_tag_name);

add_assoc_string(&tag, "tag", skipped_tag_name);
add_assoc_str(&tag, "tag", stripped_tag);
add_assoc_string(&tag, "type", "open");
add_assoc_long(&tag, "level", parser->level);

parser->ltags[parser->level-1] = estrdup(ZSTR_VAL(tag_name));
/* Because toffset may change, we should use the original tag name */
parser->ltags[parser->level - 1] = zend_string_copy(tag_name);
parser->lastwasopen = 1;

attributes = (const XML_Char **) attrs;
Expand Down Expand Up @@ -733,7 +741,7 @@ void xml_endElementHandler(void *userData, const XML_Char *name)
if (ZEND_FCC_INITIALIZED(parser->endElementHandler)) {
zval args[2];
ZVAL_COPY(&args[0], &parser->index);
ZVAL_STRING(&args[1], SKIP_TAGSTART(ZSTR_VAL(tag_name)));
ZVAL_STR(&args[1], xml_stripped_tag(tag_name, parser->toffset));

zend_call_known_fcc(&parser->endElementHandler, /* retval */ NULL, /* param_count */ 2, args, /* named_params */ NULL);
zval_ptr_dtor(&args[0]);
Expand All @@ -749,14 +757,14 @@ void xml_endElementHandler(void *userData, const XML_Char *name)
add_assoc_string(zv, "type", "complete");
}
} else {
char *skipped_tag_name = SKIP_TAGSTART(ZSTR_VAL(tag_name));
zend_string *stripped_tag = xml_stripped_tag(tag_name, parser->toffset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto


xml_add_to_info(parser, skipped_tag_name);
xml_add_to_info(parser, stripped_tag);

zval *data = xml_get_separated_data(parser);
if (EXPECTED(data)) {
array_init(&tag);
add_assoc_string(&tag, "tag", skipped_tag_name);
add_assoc_str(&tag, "tag", stripped_tag);
add_assoc_string(&tag, "type", "close");
add_assoc_long(&tag, "level", parser->level);
zend_hash_next_index_insert(Z_ARRVAL_P(data), &tag);
Expand All @@ -769,7 +777,11 @@ void xml_endElementHandler(void *userData, const XML_Char *name)
zend_string_release_ex(tag_name, 0);

if ((parser->ltags) && (parser->level <= XML_MAXLEVEL)) {
efree(parser->ltags[parser->level-1]);
zend_string **str = &parser->ltags[parser->level - 1];
if (*str) {
zend_string_release_ex(*str, false);
*str = NULL;
}
}

parser->level--;
Expand Down Expand Up @@ -868,8 +880,9 @@ void xml_characterDataHandler(void *userData, const XML_Char *s, int len)
} ZEND_HASH_FOREACH_END();
if (parser->level <= XML_MAXLEVEL && parser->level > 0 && (doprint || (! parser->skipwhite))) {
array_init(&tag);
xml_add_to_info(parser,SKIP_TAGSTART(parser->ltags[parser->level-1]));
add_assoc_string(&tag, "tag", SKIP_TAGSTART(parser->ltags[parser->level-1]));
zend_string *stripped_tag = xml_stripped_tag(parser->ltags[parser->level - 1], parser->toffset);
xml_add_to_info(parser, stripped_tag);
add_assoc_str(&tag, "tag", stripped_tag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

add_assoc_str(&tag, "value", decoded_value);
add_assoc_string(&tag, "type", "cdata");
add_assoc_long(&tag, "level", parser->level);
Expand Down Expand Up @@ -1448,8 +1461,7 @@ PHP_FUNCTION(xml_parse_into_struct)

parser->level = 0;
xml_parser_free_ltags(parser);
parser->ltags = safe_emalloc(XML_MAXLEVEL, sizeof(char *), 0);
memset(parser->ltags, 0, XML_MAXLEVEL * sizeof(char *));
parser->ltags = ecalloc(XML_MAXLEVEL, sizeof(zend_string *));

XML_SetElementHandler(parser->parser, xml_startElementHandler, xml_endElementHandler);
XML_SetCharacterDataHandler(parser->parser, xml_characterDataHandler);
Expand Down