Re: Another issue with invalid XML values

On Jul26, 2011, at 01:57 , Noah Misch wrote:
> On Mon, Jul 25, 2011 at 09:06:41PM +0200, Florian Pflug wrote:
>> On Jul25, 2011, at 20:37 , Bernd Helmle wrote:
>>> Ah, but i got now what's wrong here: configure is confusing both libxml2
>>> installations, and a quick look into config.log proves that: it uses the
>>> xml2-config from the OSX libs (my $PATH has /usr in front of the bindir of
>>> MacPorts, though i seem to recall to have changed this in the past....).
>
>> Hm, but I still think there's a bug lurking there. Using a different libxml2
>> version for the configure checks than for actual builds surely isn't good...
>>
>> From looking at configure.in, it seems that we use xml2-config to figure out
>> the CFLAGS and LDFLAGS required to build and link against libxml. I guess we
>> somehow end up not using these flags when we later test for
>> xmlStructuredErrorContext, but do use them during the actual build. Or maybe
>> the order of the -I and -L flags just ends up being different in the two cases.
>
> I can reproduce similar behavior on GNU/Linux. If my setup was sufficiently
> similar, Bernd's problematic build would have used this sequence of directives
> during both configuration and build:
>
> -I/usr/include/libxml2 -I/opt/local/include -L/opt/local/lib
>
> The directories passed using --with-includes and --with-libraries took
> priority over those from xml2-config. Since libxml2 headers live in a
> `libxml2' subdirectory, --with-includes=/opt/local/include did not affect
> finding them. --with-libraries=/opt/local/lib *did* affect finding the
> library binaries, though. Therefore, he built entirely against /usr headers
> and /opt/local libraries.

Right. Thanks for detailed analysis, Noah!

What's more, xml.c actually does attempt to protect against this situation
by calling LIBXML_TEST_VERSION in pg_xml_init_library().

But that check doesn't fire in our case, because it explicitly allows the
actual library version to be newer than the header file version, as long
as the major versions agree (the major version being "2" in this case).

So in essence, libxml promises ABI backwards-compatibility, but breaks
that promise when it comes to restoring the structured error context.

The only fix I can come up with is to explicitly test whether or not we're
able to restore the structured error context in pg_xml_init_library(). I'm
envisioning we'd so something like

xmlStructuredErrorFunc *saved_errfunc = xmlStructuredError;
#if HAVE_XMLSTRUCTUREDERRORCONTEXT
void *saved_errctx = xmlStructuredErrorContext;
#else
void *saved_errctx = xmlGenericErrorContext;
#endif

xmlSetStructuredErrorFunc((void *) MAGIC, xml_errorHandler);

#ifdef HAVE_XMLSTRUCTUREDERRORCONTEXT
if (xmlStructuredErrorContext != MAGIC)
#else
if (xmlGenericErrorContext != MAGIC)
#endif
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unable to restore the libxml error context"),
errhint("Postgres was probably built against libxml < 2.7.4 but loaded a version >= 2.7.4 at runtime")))
}

xmlSetStructuredErrorFunc(saved_errctx, saved_errfunc);

The downside of this is that a libxml version upgrade might break postgres,
of course. But by the time postgres 9.1 is released, 2.7.4 will be nearly 3
years old, so I dunno how like it is that a distro would afterwards decide to
upgrade from a version earlier than that to a version later than that.

> We could rearrange things so the xml2-config -L
> flags (or lack thereof) take priority over a --with-libraries directory for
> the purpose of finding libxml2.

Hm, how would we do that just for the purpose of finding libxml? Do autotools
provide a way to specify per-library -L flags?

> As a side note, we don't add an rpath for libxml2 like we do for Perl and
> Python. That doesn't matter on Darwin, but with GNU libc, it entails setting
> LD_LIBRARY_PATH or updating /etc/ld.so.conf to make the run time linker find
> the library binary used at build time.

Sounds like we should change that.

best regards,
Florian Pflug