Skip to content
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.

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

Allow XML entity loader for MDB2 schema loader #7498

Merged
merged 1 commit into from
Mar 3, 2014

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Mar 3, 2014

Fixes #7466

@LukasReschke @karlitschek @DeepDiver1975

I'll grep the code to find other potential cases.

@karlitschek
Copy link
Contributor

as discussed. 👍

@LukasReschke
Copy link
Member

👍 as @dor4711 confirms that this fixes the issue in #7495 (comment)

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 3, 2014

@LukasReschke there are many places in the code using simplexml_load_string which might potentially break as well...

One idea would be the re-enable entity processing directly after loading the string in 3rdparty:

  1. get the current state of entity processing
  2. call libxml_disable_entity_loader(true);
  3. make the call
  4. call libxml_disable_entity_loader(previousState);

Are you ok with this ? I'll start now while you think about it.

@LukasReschke
Copy link
Member

Are you ok with this ? I'll start now while you think about it.

Very good idea. Thanks a lot!
This seems also to be what Zend is doing with their new XML parsing library: https://github.com/zendframework/ZendXml/blob/master/library/ZendXml/Security.php

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 3, 2014

@schiesbn just pointed out that libxml_disable_entity_loader might not be thread safe, which means that while the preview XML is getting loaded, if other PHP threads are trying to work with XML and entities they might fail.

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 3, 2014

@LukasReschke looks like the Sabre part will need to be adapted upstream as well, can you report this ? Thanks.

@scrutinizer-notifier
Copy link

A new inspection was created.

@DeepDiver1975
Copy link
Member

Sabredav 1.7.11 is already taking care of this properly as far as I can tell.

We shall consider backport of 1.7.11 to stable6 for 6.0.3

Von Samsung Mobile gesendet

-------- Ursprüngliche Nachricht --------
Von: Vincent Petry notifications@github.com
Datum:03.03.2014 12:07 (GMT+01:00)
An: owncloud/core core@noreply.github.com
Cc: Thomas Müller thomas.mueller@tmit.eu
Betreff: Re: [core] Allow XML entity loader for MDB2 schema loader (#7498)

@LukasReschke looks like the Sabre part will need to be adapted upstream as well, can you report this ? Thanks.


Reply to this email directly or view it on GitHub.

@schiessle
Copy link
Contributor

👍

schiessle pushed a commit that referenced this pull request Mar 3, 2014
Allow XML entity loader for MDB2 schema loader
@schiessle schiessle merged commit dadd67e into stable6 Mar 3, 2014
@schiessle schiessle deleted the stable6-dbschemaxmlentityfix branch March 3, 2014 11:55
@PVince81
Copy link
Contributor Author

PVince81 commented Mar 3, 2014

I've prepared an upgrade of SabreDAV from upstream to stable6 for after 6.0.2 is released: owncloud-archive/3rdparty#80

And now I will forward port the libxml fixes from this PR to master

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 3, 2014

Forward port to master is here: owncloud-archive/3rdparty#81

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 3, 2014

MDB2 fix ported to master as 79ae3c4

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 3, 2014

Seems mdb2schemareader.php didn't exist on stable5, so no backport needed.
Will write up a summary of the changes in the issue ticket.

@PVince81 PVince81 mentioned this pull request Mar 3, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Aug 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants