Skip to content

Conversation

SpencerMalone
Copy link
Contributor

See: https://gitlab.gnome.org/GNOME/libxml2/-/issues/930

I was getting a segfault in a ZTS build with:

💣 Program crashed: Bad pointer dereference at 0x0000000000000038

Thread 40 "php-13" crashed:

 0 0x00000001a71a0614 xmlSchemaInitTypes + 1536 in libxml2.2.dylib
 1 0x00000001a7182fb8 xmlSchemaParse + 32 in libxml2.2.dylib
 2 _dom_document_schema_validate + 328 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/ext/dom/document.c:1787:9

  1785│                 (xmlSchemaValidityWarningFunc) php_libxml_error_handler,
  1786│                 parser);
  1787│         sptr = xmlSchemaParse(parser);                                                                                                                                        
      │         ▲
  1788│         xmlSchemaFreeParserCtxt(parser);
  1789│         PHP_LIBXML_RESTORE_GLOBALS(new_parser_ctxt);

 3 execute_ex + 132 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:57068:7

  57066│                 if (UNEXPECTED(!OPLINE)) {
  57067│ #else
  57068│                 if (UNEXPECTED((ret = ((opcode_handler_t)OPLINE->handler)(ZEND_OPCODE_HANDLER_ARGS_PASSTHRU)) != 0)) {                                                       
       │       ▲
  57069│ #endif
  57070│ #endif

 4 ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER + 520 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:2052:4

  2050│                         LOAD_OPLINE();
  2051│                         ZEND_ADD_CALL_FLAG(call, ZEND_CALL_TOP);
  2052│                         zend_execute_ex(call);                                                                                                                                
      │    ▲
  2053│                 }
  2054│         } else {

 5 execute_ex + 132 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:57068:7
 6 ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER + 520 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:2052:4
 7 execute_ex + 132 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:57068:7
 8 ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER + 520 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:2052:4
 9 execute_ex + 132 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:57068:7
10 zend_call_function + 1428 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_execute_API.c:961:3

   959│                 zend_init_func_execute_data(call, &func->op_array, fci->retval);
   960│                 ZEND_OBSERVER_FCALL_BEGIN(call);
   961│                 zend_execute_ex(call);                                                                                                                                        
      │   ▲
   962│                 EG(jit_trace_num) = orig_jit_trace_num;
   963│         } else {

11 0x00000001013bedc0 zif_frankenphp_handle_request + 900 in frankenphp
12 ZEND_DO_ICALL_SPEC_OBSERVER_HANDLER + 136 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:1400:2

  1398│ 
  1399│         zend_observer_fcall_begin(call);
  1400│         fbc->internal_function.handler(call, ret);                                                                                                                            
      │  ▲
  1401│ 
  1402│ #if ZEND_DEBUG

13 execute_ex + 132 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:57068:7
14 zend_execute.cold.1 + 344 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:61665:2

  61663│         i_init_code_execute_data(execute_data, op_array, return_value);
  61664│         ZEND_OBSERVER_FCALL_BEGIN(execute_data);
  61665│         zend_execute_ex(execute_data);                                                                                                                                       
       │  ▲
  61666│         /* Observer end handlers are called from ZEND_RETURN */
  61667│         zend_vm_stack_free_call_frame(execute_data);

15 zend_execute + 76 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend_vm_execute.h:61648:48

  61646│         }
  61647│ 
  61648│         object_or_called_scope = zend_get_this_object(EG(current_execute_data));                                                                                             
       │                                                ▲
  61649│         if (EXPECTED(!object_or_called_scope)) {
  61650│                 object_or_called_scope = zend_get_called_scope(EG(current_execute_data));

16 zend_execute_scripts + 208 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/Zend/zend.c:1895:4

  1893│                 }
  1894│                 if (op_array) {
  1895│                         zend_execute(op_array, retval);                                                                                                                       
      │    ▲
  1896│                         zend_exception_restore();
  1897│                         if (UNEXPECTED(EG(exception))) {

17 php_execute_script + 540 in libphp.dylib at /Users/spencermalone/Projects/php-src-php-8.3.25/main/main.c:2529:13

  2527│                 }
  2528│ 
  2529│                 retval = (zend_execute_scripts(ZEND_REQUIRE, NULL, 3, prepend_file_p, primary_file, append_file_p) == SUCCESS);                                               
      │             ▲
  2530│         } zend_end_try();
  2531│

Upon digging in, it looks like xmlSchemaParse is only thread safe if you call xmlSchemaInitTypes pre-threading / during init. I left out any call to xmlSchemaCleanupTypes due to 8742276, which would "normally" take care of this kind of thing, and as far as I can tell we haven't been calling cleanup in any way forever, so I'm trying to minimize changes here. If y'all think it's necessary I'm happy to adjust.

Locally, I haven't seen it crop up post-fix, but pre-fix it was occurring ~1/5th of the time when I was replicating by having https://github.com/php/frankenphp run phpunit in multiple threads.

Lemme know if I'm missing anything in making a PR here or if you want anything more rigorous than this.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Thanks!
The proper location for this would be in ext/libxml/libxml.c. The reason is that multiple extensions expose schema functionality, not just DOM.

So the solution is, more specifically, replacing the following code:

xmlInitParser();

such that it calls xmlSchemaInitTypes when libxml is compiled with schemas, and xmlInitParser (like it does now) otherwise. (You don't need the ZTS check for this). Note that xmlSchemaInitTypes will call xmlInitParser.
While at it, you can search the code for more calls to xmlSchemaInitTypes / xmlInitParser and remove them, such that the ext/libxml initialization routine is the only location.

@SpencerMalone
Copy link
Contributor Author

🫡 I can do that, I'll reply when it's ready for a re-review.

@SpencerMalone
Copy link
Contributor Author

Just pushed. Tested locally and when I did xmlInitParser in both parts of the conditional, segfault occurred immediately twice in a row, and then swapped the if to xmlSchemaInitTypes and ran w/o segfault 50 times.

@nielsdos
Copy link
Member

Thanks! That's what I actually thought of.

Just pushed. Tested locally and when I did xmlInitParser in both parts of the conditional, segfault occurred immediately twice in a row, and then swapped the if to xmlSchemaInitTypes and ran w/o segfault 50 times.

Quite interesting actually. Which version of libxml are you testing on?
I just noticed that xmlInitParser is only called from xmlSchemaInitTypes starting from libxml 2.15. So previous versions would still need the xmlInitParser call.
Are you saying the following does not work? I.e. always calling xmlInitParser unconditionally.

		xmlInitParser();
#ifdef LIBXML_SCHEMAS_ENABLED
		xmlSchemaInitTypes();
#endif

@SpencerMalone
Copy link
Contributor Author

SpencerMalone commented Oct 13, 2025

Ah, no, I'm saying for my ease of testing I was doing:

#ifdef LIBXML_SCHEMAS_ENABLED
		xmlInitParser();
#else
		xmlInitParser();
#endif

as my "pre-fix" scenario to make it easy to swap back and forth and reproduce the error + confirm the fix fixes it (in lieu of more repeatable tests).

I can swap to what you're suggesting for older versions though, I can probably get this PR updated and tested with that sometime tomorrow.

@nielsdos
Copy link
Member

Okay I see!

I can swap to what you're suggesting for older versions though, I can probably get this PR updated and tested with that sometime tomorrow.

Yeah that would be preferable after all. I should've checked the earlier versions too before making my initial suggestion.
After you push I'll test on multiple versions on ZTS to confirm.

@SpencerMalone
Copy link
Contributor Author

Pushed up, tested locally, seems happy with this changeset as well. Lemme know if you want anything else!

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

This works, thanks.
We may need more initialization calls, e.g. it seems that xmlRelaxNGInitTypes has the same issue. I'll take care of that.
On master we should remove other calls lingering around to xmlInitParser & co. I'll take care of that as well.

@nielsdos nielsdos closed this in f14e5fc Oct 14, 2025
nielsdos added a commit to nielsdos/php-src that referenced this pull request Oct 14, 2025
nielsdos added a commit that referenced this pull request Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants