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

Make libxml2 the default #2169

Closed
Dave-Allured opened this issue Dec 22, 2021 · 9 comments · Fixed by #2170
Closed

Make libxml2 the default #2169

Dave-Allured opened this issue Dec 22, 2021 · 9 comments · Fixed by #2170

Comments

@Dave-Allured
Copy link
Contributor

Version: Current development branch "main", pre-4.8.2
Platform: All
OS: All

Please make libxml2 the default in preference to ezxml. Libxml2 capability was recently added in PR #2139, but the older ezxml is selected by default, unless libxml2 is explicitly enabled.

Pros and cons are well reviewed in #2119 and #2133. For me, the most significant factors are that libxml2 is in current maintenance and monitored by many eyes for security issues. I am trying to not rehash all the previous arguments.

Ezxml in the Dec. 20 netcdf-c snapshot failed for me on Mac OS 10.15.7 Catalina and Apple clang version 12.0.0, due to #2146 (implicit declarations). With --enable-libxml2, it worked like a charm.

DennisHeimbigner added a commit to DennisHeimbigner/netcdf-c that referenced this issue Dec 23, 2021
re: PR Unidata#2139
re: PR Unidata#2169
re: PR Unidata#2146
re: Issue Unidata#2119

Found the product tinyxml2 at https://github.com/leethomason/tinyxml2.git
and replaced ezxml with it. Tinyxml2 is about twice the LOC of ezxml,
but at least is it still being maintained, and I can use it out of the box.
It is C++ rather than C, but we seem to have reached the point that we can
include C++ code with only minor compile flag changes. Untested on Mac OS.
Added instructions to the end of libncxml/Makefile.am on how to upgrade
to a later version of tinyxml2.

This PR obsoletes the use of ezxml (re PRs Unidata#2146 and https://github.com/Unidata/netcdf-c/issue/2119).
@Dave-Allured
Copy link
Contributor Author

It seems that libxml2 is falling away from current maintenance.
https://gitlab.gnome.org/GNOME/libxml2/-/issues/319

With that in combination with package size and dependencies, I am now leaning toward preferring tinyxml2 as the netcdf default parser, rather than libxml2. I would appreciate insights on this choice from others more familiar with the XML ecology. However, please do retain --enable-libxml2 as an option.

Thanks @DennisHeimbigner for already replacing the old bundled ezxml with bundled tinyxml2 (#2170).

@DennisHeimbigner
Copy link
Collaborator

I am surprised about libxml2; I thought it was the sine qua non of xml parsers.
Perhaps it has become too large and users (like me) prefer smaller footprint libraries.
Oh well, thanks for the heads up about this.

@dopplershift
Copy link
Member

Well, just because it's the most important doesn't mean the project has a lot of maintainers and support. See openssl, log4j, etc...

@Dave-Allured
Copy link
Contributor Author

I might have got the wrong impression. I was disappointed to find a maintainer problem in libxml2 recent history.

Anyway, the most important thing is to have two modern alternatives within netcdf-c. We have that now, thanks to Dennis's diligence. The choice of which one should be default, is not critical.

@e4t
Copy link
Contributor

e4t commented Jan 5, 2022

Yes, this is definitely bad. However, it seems the Gnome community is aware of this and it is discussed - I'm sure it will be resolved as well.
Let me get in touch with people involved in Gnome here at my company to see if they know more.
This may take a while as some are still gone this week.
Unfortunately, tinyxml2 has been in this state for ages. Since it doesn't have a lot of validation, the security flaws reported were just 'incarnations' of classes of issues that still exist and haven't been addressed.
I believe with libxml2 we are still better off - which should not serve as an excuse!

@Dave-Allured
Copy link
Contributor Author

@E4, please clarify. What do you mean by "tinyxml2 has been in this state for ages"? We were just talking about a maintainer problem of libxml2, not tinyxml2.

By "validation" do you mean validation against security problems? And do you think one of these packages has better validation than the other? Thank you.

@e4t
Copy link
Contributor

e4t commented Jan 5, 2022

Sorry, I mixed package names up, I meant to say 'ezxml'. Which was the original xml implementation used by netcdf-c - where security issues had started to pile up.
I do not think the present situation around libxml2 will require immediate action as I believe libxml2 is used too much to be orphaned for too long. Still, one should keep an eye on it.
I've talked to a GNOME contributor: libxml2 predates GNOME and got 'included' in GNOME due to the people involved back then.
Maybe that's the problem and makes it difficult to find a maintainer (as described in the link above). It may have to be moved to its own project, eventually.
Sorry again for the confusion.

@Dave-Allured
Copy link
Contributor Author

@e4t, I thought that was what you meant, but I needed to be certain. I am reassured by your additional remarks supporting libxml2. The original bundled ezxml is now history, so we don't need to think about that one any more.

I am still hoping to find out whether the newly-added tinyxml2 has a similar degree of security validation as libxml2.

@Dave-Allured
Copy link
Contributor Author

Dave-Allured commented Jan 14, 2022

Update. Libxml2 is back in current maintenance.
https://mail.gnome.org/archives/xml/2022-January/msg00001.html

Meanwhile, a quick look at the github site suggests that maintenance of tinyxml2 is now lagging.

Given the relative popularity of libxml2, my preference for the netCDF default flops back over to libxml2. There are other considerations. I will stop fussing about this. Unidata, do the right thing. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants