Use PCRE for our fallback regex engine when regcomp_l is unavailable by ethomson · Pull Request #4935 · libgit2/libgit2

ethomson · 2019-01-13T02:15:04Z

This removes the regex implementation that we took from glibc and replaces it with PCRE. PCRE contains a POSIX regex compatibility layer and ignores the currently running locale settings by default, which ensures that LC_COLLATE will not alter our functionality.

Now we will always use regcomp_l on systems where it is available, and when it is not, we will fallback to our builtin PCRE implementation. This means that we will never call the system regcomp which could be influenced by environment variables.

This adds the older PCRE 8.42 library, instead of the newer PCRE2 library. The original PCRE has a smaller footprint, which is nice for us, and is still supported and receiving bugfixes. Minor modifications have been made to remove the CLI components included with PCRE and to prefix the names of the POSIX compatibility functions and structures with pcre_ so that they do not collide with the system's library (if it has one).

I've also cherry-picked the tests from #4560.

ethomson · 2019-01-13T14:13:18Z

Aaaaaand we've reached that point in a C project where our circular #includes have all collapsed in on their weight.

ethomson · 2019-01-14T11:21:47Z

I broke some things out into their own header files. I'm not exactly thrilled with this in f89ab66, but 🤷‍♂️

csware · 2019-01-16T16:40:49Z

Please consider pcre2 instead of pcre.

ethomson · 2019-01-16T17:02:15Z

Please consider pcre2 instead of pcre.

I did consider it, as I mentioned in this PR description and commit messages. What's the benefit to pcre2?

tiennou · 2019-01-17T12:30:03Z

From a cursory look (FWIW PHP RFC), it seems the supported Unicode version is 10 (vs. 7), and there might be some minor behavioral differences.
There's also this thread from the git mailing-list, which seems to imply that git makes use of PCRE, so I'd be tempted to go for consistency and use that as well.

ethomson · 2019-01-25T09:42:51Z

@pks-t since you went deep on understanding the locale problems with regular expressions in #4560, I'd like your input here when you have a moment.

pks-t

I think using an external library like pcre or pcre2 is a good solution to our problem. Does the syntax match the normal POSIX syntax, though? From documentation of PCRE2 1:

When PCRE2 is called via these functions, it is only the API that is POSIX-like in style. The syntax and semantics of the regular expressions themselves are still those of Perl, subject to the setting of various PCRE2 options, as described below. "POSIX-like in style" means that the API approximates to the POSIX definition; it is not fully POSIX-compatible, and in multi-unit encoding domains it is probably even less compatible.

So to me, it doesn't sound like they really are compatible. I don't know whether the same applies to libpcre(1), too.

Otherwise, I'm not a big fan of including the dependency directly. I'd rather say that we should try to use system provided dependencies, in the following order: regcomp_l, libpcre/libpcre2, regcomp, builtin regcomp. The last two obviously still have the notable shortcomings, but developers are actually able to work around them by calling setlocale at the start of their program. And given that we didn't have any reports of that problem for such a long time, I expect that this is really only a quite limited problem. If a developer hits it, then he may just make sure to also distribute the required system libraries.

pks-t · 2019-01-25T10:09:02Z

+ * a Linking Exception. For full terms see the included COPYING file.
+ */
+#ifndef INCLUDE_posix_regex_h__
+#define INCLUDE_posix_regex_h__


Why do we use a separate header for this? Couldn't it simply be part of "posix.h"? I guess it'll get clearer in the following commits

pks-t · 2019-01-25T10:10:29Z

+#define p_regexec regexec
+#define p_regfree regfree
+
+#ifdef GIT_USE_REGCOMP_L


This is broken, right? If you directly included "posix_regex.h", this define might never be set due to the header missing an include of "common.h"->"features.h"

This has been fixed 👍

pks-t · 2019-01-25T10:20:00Z

@@ -0,0 +1,137 @@
+INCLUDE(CheckIncludeFile)


Honestly, I'm not much of a fan of including another dependency into our code which we have to keep up-to-date. I'd rather make this a compile-time option, in the following order:

libc's regcomp_l

system libpcre

system libpcre2

libc's regcomp, with the known shortcomings

built-in regcomp, with the known shortcomings

ethomson · 2019-01-25T11:27:02Z

The last two obviously still have the notable shortcomings, but developers are actually able to work around them by calling setlocale at the start of their program.

I don't feel like it's reasonable to expect developers to have to change the locale settings in order for compiled-in parts of libgit2 to work, though. I thought that was the whole point of making these changes.

Otherwise, I'm not a big fan of including the dependency directly.

We have to include some regex dependency. Today it's code from glibc that always misbehaves in some situations. If we change that dependency to be PCRE instead, we can potentially fix that issue.

Note that PCRE (v1, not PCRE2) is also git's fallback. So even if there are slight incompatibilities between GNU and PCRE, they're the same incompatibilities that git has, meaning paradoxically, that PCRE's incompatibilities keep us more compatible with git.

pks-t · 2019-01-25T12:02:24Z

The last two obviously still have the notable shortcomings, but developers are actually able to work around them by calling setlocale at the start of their program.

I don't feel like it's reasonable to expect developers to have to change the locale settings in order for compiled-in parts of libgit2 to work, though. I thought that was the whole point of making these changes.

Well, the point is giving developers at least some way of working around the issues, agreed. In some cases, they might just change locales. If that's not possible, they may decide to use this new route of using PCRE as a dependency instead by packaging it along with their application.

Otherwise, I'm not a big fan of including the dependency directly.

We have to include some regex dependency. Today it's code from glibc that always misbehaves in some situations. If we change that dependency to be PCRE instead, we can potentially fix that issue.

That we need to use some regex dependency is obvious. My point is only whether we want to bundle the dependency into our own codebase or make use of a system-provided PCRE dependency. I'm more inclined to use a system-provided one instead of bundling it with us, as otherwise we are responsible to notice and update whenever important bugfixes happen in bundled dependencies.

Note that PCRE (v1, not PCRE2) is also git's fallback. So even if there are slight incompatibilities between GNU and PCRE, they're the same incompatibilities that git has, meaning paradoxically, that PCRE's incompatibilities keep us more compatible with git.

Ah, good to know. Indeed, git recently gained the ability to also use PCRE2 instead of PCRE1.

ethomson · 2019-01-25T17:19:00Z

My point is only whether we want to bundle the dependency into our own codebase or make use of a system-provided PCRE dependency.

Hmm, okay. So what you're saying is ship the current regcomp dependency in our codebase, not ship PCRE as a dependency in our codebase instead.

You gave this list of priorities:

libc's regcomp_l
system libpcre
system libpcre2
libc's regcomp, with the known shortcomings
built-in regcomp, with the known shortcomings

It sounds like you're saying that you want to keep shipping the glibc regcomp. I guess I don't understand why shipping one dependency (the current glibc regcomp) is superior to shipping a different dependency (PCRE). We have the same problem where we are responsible for noticing and updating when important bugfixes happen, but there's no opportunity for us to fix #4528 on systems where we use our built-in regcomp (eg, Windows).

pks-t · 2019-01-28T07:09:02Z

On Fri, Jan 25, 2019 at 09:19:05AM -0800, Edward Thomson wrote: > libc's regcomp_l > system libpcre > system libpcre2 > libc's regcomp, with the known shortcomings > built-in regcomp, with the known shortcomings It sounds like you're saying that you want to keep shipping the glibc regcomp. I guess I don't understand why shipping one dependency (the current glibc `regcomp`) is superior to shipping a different dependency (PCRE). We have the same problem where we are responsible for noticing and updating when important bugfixes happen, but there's no opportunity for us to fix #4528 on systems where we use our built-in regcomp (eg, Windows).

This is indeed a very good point. I mean we could say "Let's drop regex and don't include PCRE" to make them kind of equal, but this would make the setup on Windows a lot harder. Still, my point kind of still applies: it sucks to have bundled dependencies. But I'd be happy if we simply allowed users of libgit2 to choose between system system `regcomp_l`, system `regcomp`, system PCRE or builtin PCRE. So two things I'd like to have: - We should have a compile-time switch that allows users to explicitly choose which backend to use. If no preference is given, then we should pick: system `regcomp_l`, system PCRE, builtin PCRE, in this order. Plain `regcomp` should not automatically be chosen due to the reported problems, but it might be useful to allow users to explicitly request it. - The ability to choose between either system-provided libpcre or builtin libpcre. On Linux-based distros, I expect that distributors would rather like to link against libpcre than using the bundled one.

ethomson · 2019-01-28T15:36:24Z

We should have a compile-time switch that allows users to
explicitly choose which backend to use. If no preference is
given, then we should pick: system regcomp_l, system PCRE,
builtin PCRE, in this order. Plain regcomp should not
automatically be chosen due to the reported problems, but it
might be useful to allow users to explicitly request it.

The ability to choose between either system-provided libpcre or
builtin libpcre. On Linux-based distros, I expect that
distributors would rather like to link against libpcre than
using the bundled one.

This seems like a fine plan to me.

pks-t · 2019-02-15T08:53:06Z

You need any help with regards to my proposal? Happy to help, as it's essentially my request :)

ethomson · 2019-02-17T22:09:11Z

OK, I updated this to take a -DREGEX=... option during the cmake. Options are regcomp_l (for the system regcomp_l), pcre (for a system PCRE), regcomp(for the systemregcomp) or builtin` (for the bundled PCRE).

If that is not specified during the cmake, then we will now prefer system regcomp_l, then system PCRE, and finally the builtin PCRE. If users want regcomp they will need to enable it themselves.

pks-t · 2019-02-21T13:56:18Z

+# include "win32/w32_common.h"
 # include "win32/win32-compat.h"
 # include "win32/error.h"
 # include "win32/version.h"


It might make sense to have a "win32/win32.h" header that automatically handles all win32-specific includes. This is not in the scope of this PR, though

pks-t · 2019-02-21T14:01:44Z

 #include "posix.h"
 #include "userdiff.h"

+#if LC_ALL > 0


This is a bit weird. Does Windows define LC_ALL to 0 or doesn't it define it at all? If the latter's the case, then we should definitely use #ifndef instead. I'll also upload a PR that enables -Wundef in the short-term future to generate a warning for such uses

Windows defines LC_ALL as 0.

Users can now select which regex implementation they want to use: one of the system `regcomp_l`, the system PCRE, the builtin PCRE or the system's `regcomp`. By default the system `regcomp_l` will be used if it exists, otherwise the system PCRE will be used. If neither of those exist, then the builtin PCRE implementation will be used. The system's `regcomp` is not used by default due to problems with locales.

Attempt to locate a system-installed version of PCRE and use its POSIX compatibility layer, if possible.

Use PCRE2 and its POSIX compatibility layer if requested by the user. Although PCRE2 is adequate for our needs, the PCRE2 POSIX layer as installed on Debian and Ubuntu systems is broken, so we do not opt-in to it by default to avoid breaking users on those platforms.

ethomson · 2019-05-19T10:17:26Z

OK, updated this and rebased it.

pks-t · 2019-05-21T09:03:51Z

+#define p_regexec regexec
+#define p_regfree regfree
+
+#ifdef GIT_USE_REGCOMP_L


This has been fixed 👍

pks-t · 2019-05-21T09:18:37Z

 OPTION(ENABLE_WERROR			"Enable compilation with -Werror"			OFF)
 OPTION(USE_BUNDLED_ZLIB    		"Use the bundled version of zlib"			OFF)
 OPTION(DEPRECATE_HARD			"Do not include deprecated functions in the library"	OFF)
+   SET(REGEX				"" CACHE STRING "Regular expression implementation. One of regcomp_l, pcre, regcomp, or builtin.")


This variable name may be a bit problematic, as it is a keyword in CMake. We might want to rename it. "REGEX_BACKEND" would come to my mind, as there's precedent for this already, even though I seem to recall that you don't like the name "backend"

I'm fine with regex backend (my avoidance of "backend" comes from using it without qualification. That is I'm fine with having a regex backend but to talk about a "backend" as something concrete sounds odd to me.)

pks-t · 2019-05-21T09:21:18Z

 */

-#ifdef GIT_REGEX_PCRE
+#ifdef GIT_REGEX_BUILTIN


Huh? Where do we define p_regex_ stuff for GIT_REGEX_PCRE now?

Edit: never mind, the following commit clears that up

pks-t · 2019-05-21T09:24:48Z

 IF(REGEX STREQUAL "regcomp_l")
 	ADD_FEATURE_INFO(regex ON "using system regcomp_l")
 	SET(GIT_REGEX_REGCOMP_L 1)
+ELSEIF(REGEX STREQUAL "pcre2")


Any reason why we shouldn't auto-detect the PCRE2 library IF(REGEX STREQUAL "")?

Hmm, I tried to mention this in a comment, but it looks like it got buried. (It is mentioned in the commit that introduces PCRE2.)

Basically, the POSIX compatibility layer for PCRE2 is completely broken on Ubuntu and Debian. If you look at PCRE, you'll see that it has symbols of pcre_regcomp, and then pcreposix.h sets up that indirection as #define regcomp pcre_regcomp. This avoids conflicting with the regex functions that are in libc.

The PCRE2 packages only do half of this, they have symbols for PCRE2regcomp but they forgot to set up the indirection in the header files. (They just declare regcomp.) So you end up using the regular libc regex bits. Worse, you use the pcre2 header files and their #defines for constants, so you end up passing the wrong constants to libc, and things go very wrong.

So we allow people to set PCRE2 manually, in case they have a working installation, but we don't autodetect it to avoid breaking Debian/Ubuntu users.

Hm. Couldn't we just use CMake to detect whether the system PCRE2 is broken or not with CHECK_FUNCTION_EXISTS?

Hmm, maybe yes, but since they haven't fixed it yet, I don't even have a test case that I can verify that we're getting it correct (yet).

My preference would be to hold off until a (shipping) version of Debian/Ubuntu has this fixed and we can test it...

Fair enough. By holding off you probably just refer to the proper detection of non-broken PCRE2, right?

Yeah, keep it as-is where people can explicitly opt-in to PCRE2 support but not enable PCRE2 by default (yet).

For the record, pcre2 has added compatibility aliases (#define regexec pcre2_regexec) in 10.33 (in r1064).

ethomson · 2019-05-21T09:47:49Z

I'll update REGEX in the cmake to REGEX_BACKEND. Are you happy with me 🚢 this once I do?

pks-t · 2019-05-21T10:15:04Z

I'm still a bit hesitant with regards to the PCRE2 bits, but this is really nothing that should hold back this PR as we do not loose any functionality at all. So: go for it!

ethomson · 2019-05-21T10:18:10Z

Cool. I want to get this merged so that we can stomp on the warnings in Windows (a lot of them come from the existing regex implementation). We can iterate on the PCRE2 support.

This avoids any misunderstanding with the REGEX keyword in cmake.

ethomson · 2019-05-21T10:27:21Z

Updated to use REGEX_BACKEND as the option.

pks-t · 2019-05-21T10:42:48Z

By the way, just came to my mind: should we update Azure for this? Like, having at least one build explicitly using bundled PCRE and regcomp_l, respectively?

tiennou · 2019-05-21T11:02:52Z

(relatedly, macOS also has both the POSIX2008.smth regcomp_l and pcre1/2 (in Homebrew), so it might be somewhat easier to test the build system)

Explicitly enable the `builtin` regex backend and the PCRE backend for some Linux builds.

ethomson · 2019-05-21T12:42:25Z

OK, I've added PCRE to the trusty docker image, and set some explicit regex configuration during builds. (One Linux build explicitly sets PCRE, one explicitly sets builtin, and now the macOS built explicitly sets regcomp_l.)

pks-t · 2019-05-21T12:57:24Z

OK, I've added PCRE to the trusty docker image, and set some explicit regex configuration during builds. (One Linux build explicitly sets PCRE, one explicitly sets builtin, and now the macOS built explicitly sets regcomp_l.)

Perfect, thanks! No more remarks from my side now, I promise.

ethomson · 2019-05-21T12:59:50Z

Perfect, thanks! No more remarks from my side now, I promise.

In that case, 🚢 . Thanks for the review everyone!

libgit2 has bundled pcre (not pcre2) in libgit2/libgit2#4935 Before this change configure printed "regex, using bundled PCRE", after this change it prints "regex, using system PCRE".

ethomson force-pushed the ethomson/pcre branch 4 times, most recently from 589ed7b to 5cbf617 Compare January 13, 2019 11:02

ethomson force-pushed the ethomson/pcre branch 2 times, most recently from 232611c to 3883800 Compare January 13, 2019 21:20

pks-t reviewed Jan 25, 2019

View reviewed changes

ethomson mentioned this pull request Jan 25, 2019

1.0 planning / next few releases #4960

Closed

6 tasks

ethomson added the 1.0 label Feb 14, 2019

ethomson force-pushed the ethomson/pcre branch from 3883800 to a4c5336 Compare February 14, 2019 23:50

pks-t mentioned this pull request Feb 15, 2019

Regcomp with LC_COLLATE #4560

Closed

ethomson force-pushed the ethomson/pcre branch from 56639a2 to c94cb30 Compare February 17, 2019 22:01

pks-t reviewed Feb 21, 2019

View reviewed changes

pks-t mentioned this pull request Feb 25, 2019

cmake: correctly detect if system provides regcomp #5007

Closed

ethomson force-pushed the ethomson/pcre branch 3 times, most recently from 6cef68c to 8885931 Compare May 19, 2019 09:37

ethomson added 4 commits May 19, 2019 11:12

regex: disambiguate builtin vs system pcre

622166c

regex: use system PCRE if available

69ecdad

Attempt to locate a system-installed version of PCRE and use its POSIX compatibility layer, if possible.

ethomson force-pushed the ethomson/pcre branch from 8885931 to ce6d624 Compare May 19, 2019 10:12

pks-t reviewed May 21, 2019

View reviewed changes

regex: use REGEX_BACKEND as the cmake option name

ac2b235

This avoids any misunderstanding with the REGEX keyword in cmake.

ci: use a mix of regex backends

afb04a9

Explicitly enable the `builtin` regex backend and the PCRE backend for some Linux builds.

ethomson force-pushed the ethomson/pcre branch from 5e0c096 to afb04a9 Compare May 21, 2019 12:26

ethomson merged commit 3d9e82f into master May 21, 2019

ethomson mentioned this pull request May 21, 2019

git_diff_get_stats fails when LC_ALL="" #4528

Closed

implausible mentioned this pull request Jun 26, 2019

"Invalid collation character" when trying to diff nodegit/nodegit#1097

Open

ehuss mentioned this pull request Jul 15, 2019

Update libgit2 to latest master. rust-lang/git2-rs#440

Merged

ethomson deleted the ethomson/pcre branch February 2, 2020 13:08

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

orivej mentioned this pull request Apr 12, 2020

libgit2: unbundle pcre NixOS/nixpkgs#85049

Merged

10 tasks

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Conversation

ethomson commented Jan 13, 2019

Uh oh!

ethomson commented Jan 13, 2019

Uh oh!

ethomson commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csware commented Jan 16, 2019

Uh oh!

ethomson commented Jan 16, 2019

Uh oh!

tiennou commented Jan 17, 2019

Uh oh!

ethomson commented Jan 25, 2019

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ethomson commented Jan 25, 2019

Uh oh!

pks-t commented Jan 25, 2019

Uh oh!

ethomson commented Jan 25, 2019

Uh oh!

pks-t commented Jan 28, 2019 via email

Uh oh!

ethomson commented Jan 28, 2019

Uh oh!

pks-t commented Feb 15, 2019

Uh oh!

ethomson commented Feb 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ethomson commented May 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ethomson commented Jan 14, 2019 •

edited

Loading

ethomson commented Feb 17, 2019 •

edited

Loading

tiennou commented May 21, 2019 •

edited

Loading