Re: Making test framework and trunk approachable (was: PCRE 10 and puzzling edge cases)

[Petr Pisar]

On Mon, Dec 12, 2016 at 01:03:23PM -0600, William A Rowe Jr wrote:
> System APR does require the -devel package. That RHEL package aught to
> install the .m4 logic required by httpd, but it is on us to find that in a
> conventional place.
> 
I think this should be handled in the buildconf script. The apr-devel package
puts find_apr.m4 into /usr/share/aclocal (check "aclocal --print-ac-dir"
output).

> Httpd trunk is already hunting for yet unreleased APR 1.6 features, but we
> should better document that.
> 
That can happen. Then there is nothing to improve.

> Not yet installed httpd is a little more complex, maybe we can add an
> interim apxs in the build tree to facilitate this?
>
Something like libtool allows to run executables linked to just-built
libraries. Maybe.

But I think the hardest issue is the modules sources are scattered across many
directories and the just-built module DSOs too. Tests should have to be
smarter in locating them.

-- Petr


signature.asc
Description: PGP signature

Making test framework and trunk approachable (was: PCRE 10 and puzzling edge cases)

[William A Rowe Jr]

On Dec 12, 2016 3:52 AM, "Petr Pisar"  wrote:


I think the inability to build httpd against system APR and to run test
against not yet installed httpd is quite surprising.


Thanks for taking the additional time to document your experiences!

System APR does require the -devel package. That RHEL package aught to
install the .m4 logic required by httpd, but it is on us to find that in a
conventional place.

Httpd trunk is already hunting for yet unreleased APR 1.6 features, but we
should better document that.

Not yet installed httpd is a little more complex, maybe we can add an
interim apxs in the build tree to facilitate this?

Working from my hardcopy of your list once our current release effort is
finished. Again, thanks.

Re: PCRE 10 and puzzling edge cases

[Reindl Harald]

Am 12.12.2016 um 10:52 schrieb Petr Pisar:

I made sure I have installed all Perl modules I found relevant, but I was
unable to run the tests against SVN httpd sources. I played with
LD_LIBRARY_PATH, apxs etc. but without any good result. At the end
I reconfigured httpd sources and installed the binaries into a /tmp
subdirectory and that allowed me to run the tests. It's quite annoying to have
to install httpd into the system just only to test it


since your email is @redhat.com take a look at the Fedora php.spec and 
how the testsuite is called there from the rpm builddir *before* make 
install - same for mariadb/mysql - i guess httpd wouldn't bee that different

Re: PCRE 10 and puzzling edge cases

[Petr Pisar]

On Fri, Dec 09, 2016 at 01:07:54PM -0600, William A Rowe Jr wrote:
> 
> Thanks again, if there is anything that we didn't document well about
> getting the tests to run, we would like to fix that and make it easier for
> developers to cobble together their own test environment. We certainly don't
> want it to take hours for an experienced newcomer to get from point a to
> point b.
> 
First problem was how to generate ./configure from SVN sources. Obvious
autoreconf or autoconf did not work because of some missing macros. Then
I found ./buildconf.

But it still failed because of missing APR sources despite I had installed
APR 1.5.4 including header files and M4 macros from my distribution packages.
Then I found in the documentation that building from SVN requires APR sources.
So I cloned APR sources. Then I was able to build httpd.

I wanted to run test, but there are no tests you complained about a failure.
After searching web, I got an idea that httpd-framework is what I need.

I made sure I have installed all Perl modules I found relevant, but I was
unable to run the tests against SVN httpd sources. I played with
LD_LIBRARY_PATH, apxs etc. but without any good result. At the end
I reconfigured httpd sources and installed the binaries into a /tmp
subdirectory and that allowed me to run the tests. It's quite annoying to have
to install httpd into the system just only to test it.

But still the tests failed because of an undefined symbol in a session test
module. The symbol was defined in the httpd session module but the session
module was not named by the LoadModule keyword in the generated
t/conf/httpd.conf. I did not found a way how to disable the session tests
other than removing c-modules/test_session/mod_test_session.c file.

Finally I obtained a pass.

I think the inability to build httpd against system APR and to run test
against not yet installed httpd is quite surprising.

-- Petr


signature.asc
Description: PGP signature

Re: PCRE 10 and puzzling edge cases

[William A Rowe Jr]

On Fri, Dec 9, 2016 at 8:05 AM, Petr Pisar  wrote:

> On Thu, Dec 08, 2016 at 11:09:42AM -0600, William A Rowe Jr wrote:
> > I've beaten my head against this wall for a bit longer, and came up with
> > several places where pcre2 changed return types for void *what query
> > interogations (especially from int to uint32, badness on x86_64-linux).
> >
> > The attached patch picks up these bad void * type assignments. Still
> > no tremendous improvement, missing something blatantly obvious,
> > I expect.
> >
> After few hours of getting httpd sources and tests and hacking them to
> actually obtain a pass, I applied your patch and looked what's wrong with
> your
> PCRE2 code.
>

Thanks again, if there is anything that we didn't document well about
getting
the tests to run, we would like to fix that and make it easier for
developers
to cobble together their own test environment. We certainly don't want it
to take hours for an experienced newcomer to get from point a to point b.

Happy to improve this based on your observations.

The t/apache/expr.t failed because the pcre2_match() alwayes returned -51
> that means PCRE2_ERROR_NULL. The reason is you used the old PCRE
> optimization
> path and called pcre2_match_data_create(nmatch, NULL) only if nmatch was
> positive. As a result, pcre2_match_data_create() was never called, so you
> passed NULL matchdata to pcre2_match(), hence the failure.
>

Yup, that was a bad assumption on my part, reading pcre2api in parallel
with pcreapi.


> The tests still do not pass, but that's probably another (PCRE2) problem.
> I hope I helped you at lest somehow.
>

I do have it working, now committed. Failing tests are likely some missing
cpan modules in your setup still, or perhaps some tests that are making
bogus assumptions about the supported dependency libraries. Thanks for
the pointer, we seem to have succeeded!

Re: PCRE 10 and puzzling edge cases

[William A Rowe Jr]

On Dec 9, 2016 8:06 AM, "Petr Pisar"  wrote:

On Thu, Dec 08, 2016 at 11:09:42AM -0600, William A Rowe Jr wrote:
> I've beaten my head against this wall for a bit longer, and came up with
> several places where pcre2 changed return types for void *what query
> interogations (especially from int to uint32, badness on x86_64-linux).
>
> The attached patch picks up these bad void * type assignments. Still
> no tremendous improvement, missing something blatantly obvious,
> I expect.
>
After few hours of getting httpd sources and tests and hacking them to
actually obtain a pass, I applied your patch and looked what's wrong with
your
PCRE2 code.

The t/apache/expr.t failed because the pcre2_match() alwayes returned -51
that means PCRE2_ERROR_NULL. The reason is you used the old PCRE
optimization
path and called pcre2_match_data_create(nmatch, NULL) only if nmatch was
positive. As a result, pcre2_match_data_create() was never called, so you
passed NULL matchdata to pcre2_match(), hence the failure.

See the attached fix.

The tests still do not pass, but that's probably another (PCRE2) problem.
I hope I helped you at lest somehow


That's a huge help, thanks Petr.

I'm also curious along that path if adding PCRE2 (or PCRE) study pattern
would also be helpful. Something to experiment with once both code paths
are working.

Cheers,

Bill

Re: PCRE 10 and puzzling edge cases

[Petr Pisar]

On Thu, Dec 08, 2016 at 11:09:42AM -0600, William A Rowe Jr wrote:
> I've beaten my head against this wall for a bit longer, and came up with
> several places where pcre2 changed return types for void *what query
> interogations (especially from int to uint32, badness on x86_64-linux).
> 
> The attached patch picks up these bad void * type assignments. Still
> no tremendous improvement, missing something blatantly obvious,
> I expect.
> 
After few hours of getting httpd sources and tests and hacking them to
actually obtain a pass, I applied your patch and looked what's wrong with your
PCRE2 code.

The t/apache/expr.t failed because the pcre2_match() alwayes returned -51
that means PCRE2_ERROR_NULL. The reason is you used the old PCRE optimization
path and called pcre2_match_data_create(nmatch, NULL) only if nmatch was
positive. As a result, pcre2_match_data_create() was never called, so you
passed NULL matchdata to pcre2_match(), hence the failure.

See the attached fix.

The tests still do not pass, but that's probably another (PCRE2) problem.
I hope I helped you at lest somehow.

-- Petr
From 5e28aa58b19fdbfe485f50668c3176fe23e25609 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= 
Date: Fri, 9 Dec 2016 14:59:57 +0100
Subject: [PATCH] Fix never match
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PCRE2 always requires pcre2_match_data_create(). Otherwise
pcre2_match() returns PCRE2_ERROR_NULL.

Signed-off-by: Petr Písař 
---
 server/util_pcre.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/server/util_pcre.c b/server/util_pcre.c
index 845dd33..0c435ff 100644
--- a/server/util_pcre.c
+++ b/server/util_pcre.c
@@ -248,13 +248,13 @@ AP_DECLARE(int) ap_regexec_len(const ap_regex_t *preg, 
const char *buff,
 
 ((ap_regex_t *)preg)->re_erroffset = (apr_size_t)(-1);/* Only has 
meaning after compile */
 
-if (nmatch > 0) {
 #ifdef HAVE_PCRE2
-matchdata = pcre2_match_data_create(nmatch, NULL);
-if (matchdata == NULL)
-return AP_REG_ESPACE;
-ovector = pcre2_get_ovector_pointer(matchdata);
+matchdata = pcre2_match_data_create(nmatch, NULL);
+if (matchdata == NULL)
+return AP_REG_ESPACE;
+ovector = pcre2_get_ovector_pointer(matchdata);
 #else
+if (nmatch > 0) {
 if (nmatch <= POSIX_MALLOC_THRESHOLD) {
 ovector = &(small_ovector[0]);
 }
@@ -264,8 +264,8 @@ AP_DECLARE(int) ap_regexec_len(const ap_regex_t *preg, 
const char *buff,
 return AP_REG_ESPACE;
 allocated_ovector = 1;
 }
-#endif
 }
+#endif
 
 #ifdef HAVE_PCRE2
 rc = pcre2_match((const pcre2_code *)preg->re_pcre,
@@ -290,8 +290,7 @@ AP_DECLARE(int) ap_regexec_len(const ap_regex_t *preg, 
const char *buff,
 }
 
 #ifdef HAVE_PCRE2
-if (matchdata)
-pcre2_match_data_free(matchdata);
+pcre2_match_data_free(matchdata);
 #else
 if (allocated_ovector)
 free(ovector);
-- 
2.7.4



signature.asc
Description: PGP signature

Re: PCRE 10 and puzzling edge cases

[William A Rowe Jr]

I've beaten my head against this wall for a bit longer, and came up with
several places where pcre2 changed return types for void *what query
interogations (especially from int to uint32, badness on x86_64-linux).

The attached patch picks up these bad void * type assignments. Still
no tremendous improvement, missing something blatantly obvious,
I expect.



On Mon, Dec 5, 2016 at 10:59 PM, William A Rowe Jr 
wrote:

> I've written the following patch to trunk to allow us to configure,
> compile and link against PCRE2 (10.x). The autoconf in particular is
> streamlined for cross-compilation detection, while retaining the ability to
> override the path to (and name of) pcre[2]-config.
>
> It isn't in a commit-ready state due to t/TEST t/apache/expr.t failures
> (among others), and the defects appear to revolve around the way substring
> patterns are recorded.
>
> Attached the test failure cases (many similar test patterns do succeed,
> interestingly.) One test looks outright wrong. I'd rather not beat my head
> against these if the answer is blatantly obvious.
>
> If anyone has patience for exploring this further, any help is welcomed.
> Philip starts with this assertion; "The original, very widely deployed PCRE
> library, originally released in 1997, is at version 8.39, and the API and
> feature set are stable—future releases will be for bugfixes only. All new
> future features will be to PCRE2, not the original PCRE 8.x series." But he
> has gone on to state that many fuzzing error cases which are handled
> correctly in PCRE2 cannot be realistically fixed in PCRE 8.x. I've placed
> this up there with other parsing rewrites in httpd, that starting over is
> simply the correct answer, and I'd like to see if we can have httpd 3.0
> choosing PCRE2 over PCRE in the near future (and perhaps backport this if
> we determine behavior is consistent.)
>
> Cheers,
>
> Bill
>
>
Index: configure.in
===
--- configure.in	(revision 1773161)
+++ configure.in	(working copy)
@@ -223,18 +223,18 @@
 AC_ARG_WITH(pcre,
 APACHE_HELP_STRING(--with-pcre=PATH,Use external PCRE library))
 
-AC_PATH_PROG(PCRE_CONFIG, pcre-config, false)
-if test -d "$with_pcre" && test -x "$with_pcre/bin/pcre-config"; then
-   PCRE_CONFIG=$with_pcre/bin/pcre-config
-elif test -x "$with_pcre"; then
-   PCRE_CONFIG=$with_pcre
-fi
+AC_CHECK_TARGET_TOOLS(PCRE_CONFIG, [pcre2-config pcre-config],
+  [`which $with_pcre 2>/dev/null`],
+  [$with_pcre/bin:$with_pcre])
 
-if test "$PCRE_CONFIG" != "false"; then
+if test "x$PCRE_CONFIG" != "x"; then
   if $PCRE_CONFIG --version >/dev/null 2>&1; then :; else
-AC_MSG_ERROR([Did not find pcre-config script at $PCRE_CONFIG])
+AC_MSG_ERROR([Did not find working script at $PCRE_CONFIG])
   fi
   case `$PCRE_CONFIG --version` in
+  [1[0-9].*])
+AC_DEFINE(HAVE_PCRE2, 1, [Detected PCRE2]) 
+;;
   [[1-5].*])
 AC_MSG_ERROR([Need at least pcre version 6.7])
 ;;
@@ -244,10 +244,10 @@
   esac
   AC_MSG_NOTICE([Using external PCRE library from $PCRE_CONFIG])
   APR_ADDTO(PCRE_INCLUDES, [`$PCRE_CONFIG --cflags`])
-  APR_ADDTO(PCRE_LIBS, [`$PCRE_CONFIG --libs`])
+  APR_ADDTO(PCRE_LIBS, [`$PCRE_CONFIG --libs8 2>/dev/null || $PCRE_CONFIG --libs`])
   APR_ADDTO(HTTPD_LIBS, [\$(PCRE_LIBS)])
 else
-  AC_MSG_ERROR([pcre-config for libpcre not found. PCRE is required and available from http://pcre.org/])
+  AC_MSG_ERROR([pcre(2)-config for libpcre not found. PCRE is required and available from http://pcre.org/])
 fi
 APACHE_SUBST(PCRE_LIBS)
 
Index: server/util_pcre.c
===
--- server/util_pcre.c	(revision 1773161)
+++ server/util_pcre.c	(working copy)
@@ -46,10 +46,18 @@
 #include "httpd.h"
 #include "apr_strings.h"
 #include "apr_tables.h"
+
+#ifdef HAVE_PCRE2
+#define PCRE2_CODE_UNIT_WIDTH 8
+#include "pcre2.h"
+#define PCREn(x) PCRE2_ ## x
+#else
 #include "pcre.h"
+#define PCREn(x) PCRE_ ## x
+#endif
 
 /* PCRE_DUPNAMES is only present since version 6.7 of PCRE */
-#ifndef PCRE_DUPNAMES
+#if !defined(PCRE_DUPNAMES) && !defined(HAVE_PCRE2)
 #error PCRE Version 6.7 or later required!
 #else
 
@@ -74,11 +82,19 @@
 
 AP_DECLARE(const char *) ap_pcre_version_string(int which)
 {
+#ifdef HAVE_PCRE2
+static char buf[80];
+#endif
 switch (which) {
 case AP_REG_PCRE_COMPILED:
-return APR_STRINGIFY(PCRE_MAJOR) "." APR_STRINGIFY(PCRE_MINOR) " " APR_STRINGIFY(PCRE_DATE);
+return APR_STRINGIFY(PCREn(MAJOR)) "." APR_STRINGIFY(PCREn(MINOR)) " " APR_STRINGIFY(PCREn(DATE));
 case AP_REG_PCRE_LOADED:
+#ifdef HAVE_PCRE2
+pcre2_config(PCRE2_CONFIG_VERSION, buf);
+return buf;
+#else
 return pcre_version();
+#endif
 default:
 return "Unknown";
 }
@@ -118,7 +134,11 @@
 
 AP_DECLARE(void) ap_regfree(ap_regex_t *preg)
 {
+#ifdef HAVE_PCRE2
+pcre2_code_free(preg->re_pcre);

PCRE 10 and puzzling edge cases

[William A Rowe Jr]

I've written the following patch to trunk to allow us to configure, compile
and link against PCRE2 (10.x). The autoconf in particular is streamlined
for cross-compilation detection, while retaining the ability to override
the path to (and name of) pcre[2]-config.

It isn't in a commit-ready state due to t/TEST t/apache/expr.t failures
(among others), and the defects appear to revolve around the way substring
patterns are recorded.

Attached the test failure cases (many similar test patterns do succeed,
interestingly.) One test looks outright wrong. I'd rather not beat my head
against these if the answer is blatantly obvious.

If anyone has patience for exploring this further, any help is welcomed.
Philip starts with this assertion; "The original, very widely deployed PCRE
library, originally released in 1997, is at version 8.39, and the API and
feature set are stable—future releases will be for bugfixes only. All new
future features will be to PCRE2, not the original PCRE 8.x series." But he
has gone on to state that many fuzzing error cases which are handled
correctly in PCRE2 cannot be realistically fixed in PCRE 8.x. I've placed
this up there with other parsing rewrites in httpd, that starting over is
simply the correct answer, and I'd like to see if we can have httpd 3.0
choosing PCRE2 over PCRE in the near future (and perhaps backport this if
we determine behavior is consistent.)

Cheers,

Bill
Index: configure.in
===
--- configure.in	(revision 1772810)
+++ configure.in	(working copy)
@@ -223,18 +223,18 @@
 AC_ARG_WITH(pcre,
 APACHE_HELP_STRING(--with-pcre=PATH,Use external PCRE library))
 
-AC_PATH_PROG(PCRE_CONFIG, pcre-config, false)
-if test -d "$with_pcre" && test -x "$with_pcre/bin/pcre-config"; then
-   PCRE_CONFIG=$with_pcre/bin/pcre-config
-elif test -x "$with_pcre"; then
-   PCRE_CONFIG=$with_pcre
-fi
+AC_CHECK_TARGET_TOOLS(PCRE_CONFIG, [pcre2-config pcre-config],
+  [`which $with_pcre 2>/dev/null`],
+  [$with_pcre/bin:$with_pcre])
 
-if test "$PCRE_CONFIG" != "false"; then
+if test "x$PCRE_CONFIG" != "x"; then
   if $PCRE_CONFIG --version >/dev/null 2>&1; then :; else
-AC_MSG_ERROR([Did not find pcre-config script at $PCRE_CONFIG])
+AC_MSG_ERROR([Did not find working script at $PCRE_CONFIG])
   fi
   case `$PCRE_CONFIG --version` in
+  [1[0-9].*])
+AC_DEFINE(HAVE_PCRE2, 1, [Detected PCRE2]) 
+;;
   [[1-5].*])
 AC_MSG_ERROR([Need at least pcre version 6.7])
 ;;
@@ -244,10 +244,10 @@
   esac
   AC_MSG_NOTICE([Using external PCRE library from $PCRE_CONFIG])
   APR_ADDTO(PCRE_INCLUDES, [`$PCRE_CONFIG --cflags`])
-  APR_ADDTO(PCRE_LIBS, [`$PCRE_CONFIG --libs`])
+  APR_ADDTO(PCRE_LIBS, [`$PCRE_CONFIG --libs8 2>/dev/null || $PCRE_CONFIG --libs`])
   APR_ADDTO(HTTPD_LIBS, [\$(PCRE_LIBS)])
 else
-  AC_MSG_ERROR([pcre-config for libpcre not found. PCRE is required and available from http://pcre.org/])
+  AC_MSG_ERROR([pcre(2)-config for libpcre not found. PCRE is required and available from http://pcre.org/])
 fi
 APACHE_SUBST(PCRE_LIBS)
 
Index: server/util_pcre.c
===
--- server/util_pcre.c	(revision 1772810)
+++ server/util_pcre.c	(working copy)
@@ -46,10 +46,18 @@
 #include "httpd.h"
 #include "apr_strings.h"
 #include "apr_tables.h"
+
+#ifdef HAVE_PCRE2
+#define PCRE2_CODE_UNIT_WIDTH 8
+#include "pcre2.h"
+#define PCREn(x) PCRE2_ ## x
+#else
 #include "pcre.h"
+#define PCREn(x) PCRE_ ## x
+#endif
 
 /* PCRE_DUPNAMES is only present since version 6.7 of PCRE */
-#ifndef PCRE_DUPNAMES
+#if !defined(PCRE_DUPNAMES) && !defined(HAVE_PCRE2)
 #error PCRE Version 6.7 or later required!
 #else
 
@@ -74,11 +82,19 @@
 
 AP_DECLARE(const char *) ap_pcre_version_string(int which)
 {
+#ifdef HAVE_PCRE2
+static char buf[80];
+#endif
 switch (which) {
 case AP_REG_PCRE_COMPILED:
-return APR_STRINGIFY(PCRE_MAJOR) "." APR_STRINGIFY(PCRE_MINOR) " " APR_STRINGIFY(PCRE_DATE);
+return APR_STRINGIFY(PCREn(MAJOR)) "." APR_STRINGIFY(PCREn(MINOR)) " " APR_STRINGIFY(PCREn(DATE));
 case AP_REG_PCRE_LOADED:
+#ifdef HAVE_PCRE2
+pcre2_config(PCRE2_CONFIG_VERSION, buf);
+return buf;
+#else
 return pcre_version();
+#endif
 default:
 return "Unknown";
 }
@@ -118,7 +134,11 @@
 
 AP_DECLARE(void) ap_regfree(ap_regex_t *preg)
 {
-(pcre_free)(preg->re_pcre);
+#ifdef HAVE_PCRE2
+pcre2_code_free(preg->re_pcre);
+#else
+pcre_free(preg->re_pcre);
+#endif
 }
 
 
@@ -139,34 +159,46 @@
 */
 AP_DECLARE(int) ap_regcomp(ap_regex_t * preg, const char *pattern, int cflags)
 {
+#ifdef HAVE_PCRE2
+size_t erroffset;
+#else
 const char *errorptr;
 int erroffset;
+#endif
 int errcode = 0;
-int options = PCRE_DUPNAMES;
+int options = PCREn(DUPNAMES);
 
 if ((cflags & AP_REG_ICASE) != 0)
-options |= PCRE_CASELESS;
+options |=