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

[Core] Move project to PCRE2 #2299

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ansuel
Copy link

@Ansuel Ansuel commented Nov 3, 2023

Move project to PCRE2 as PCRE is EOL and won't receive any security
updates anymore.

PCRE2 have different API compared to PCRE. Mainly PCRE2 have the concept
of match_data, no ovector needs to be passed, different handling for
error string and different handling for substring manipulation.

Update any user of PCRE library with the new API

Signed-off-by: Christian Marangi [email protected]


This also have 2 bugfix that i found while doing the conversion..

Codewise I should have ported anything... What I need help is in the visual studio project files and some other small thing... Hope someone can help with them!

@Ansuel Ansuel changed the title Pcre2 conversion [Core] Move project to PCRE2 Nov 3, 2023
@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@Ansuel Ansuel force-pushed the pcre2-conversion branch 4 times, most recently from 8506bdf to 1b8ccba Compare November 3, 2023 20:59
@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@Ansuel Ansuel force-pushed the pcre2-conversion branch 2 times, most recently from 42162a8 to 6b59763 Compare November 3, 2023 22:31
@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@Ansuel
Copy link
Author

Ansuel commented Nov 3, 2023

Well took me a bit to fix all the hiccups so sorry for the mass force push and stuff... Problem with windows related thing still apply...

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@signalwire-ci
Copy link

signalwire-ci bot commented Nov 3, 2023

@Ansuel
Copy link
Author

Ansuel commented Apr 12, 2024

@andywolk any news with this?

@Ansuel
Copy link
Author

Ansuel commented Apr 27, 2024

@dragos-oancea @andywolk Any chance someone can look at this? This is one of the few remaining package that still depends on PCRE and we are trying to drop PCRE on openwrt.

Most of the changes as simple and straight migration from PCRE to PCRE2.

@BKPepe
Copy link

BKPepe commented May 11, 2024

Guys, kind reminder. 2 weeks passed. This PR is here since 3rd November.

@jakubkarolczyk Any chance to review this, please?

@dannil
Copy link

dannil commented Jun 23, 2024

Ping

@BKPepe
Copy link

BKPepe commented Jul 20, 2024

Guys (@s3rj1k , @andywolk) would you mind to take a look at this pull request, please?

@andywolk andywolk self-requested a review November 5, 2024 17:25
@andywolk
Copy link
Contributor

andywolk commented Nov 5, 2024

That first commit

[mod_verto] Fix memory leak by correctly freeing regex

could probably go as a separate fix regardless.

@andywolk
Copy link
Contributor

andywolk commented Nov 5, 2024

I could also reswig master so this PR does not have to have that swig commit.

@andywolk
Copy link
Contributor

andywolk commented Nov 5, 2024

The swig commit in this PR is not required.

For mod_verto regex was never freed and was actually leaking memory.
Correctly free the compiled regex to fix the memory leak.

Signed-off-by: Christian Marangi <[email protected]>
Move project to PCRE2 as PCRE is EOL and won't receive any security
updates anymore.

PCRE2 have different API compared to PCRE. Mainly PCRE2 have the concept
of match_data, no ovector needs to be passed, different handling for
error string and different handling for substring manipulation.

Update any user of PCRE library with the new API

Signed-off-by: Christian Marangi <[email protected]>
@Ansuel
Copy link
Author

Ansuel commented Nov 5, 2024

@andywolk I split the fix commit in a separate PR. I will drop the commit from here once it's merged. Thanks a lot for the followup

@Ansuel
Copy link
Author

Ansuel commented Nov 8, 2024

@andywolk BTW is also produced this alternative version Ansuel@60ad581

This doesn't cause ANY change in the current API but have some hacks in mind:

  • We need to copy the internal struct of pcre2
    New PCRE2 is all around match_data struct. Everything after a pcre2_match is put here. Original PCRE relay on ovector. We can mimik this by composing our own match_data but internal values are not exposed (hence having a copy of the struct in our header is needed)
  • We need to allocate a big struct for the error strings
    PCRE2 dropped giving direct string and just return a code and provide an helper to copy the string in an allocated buffer.

Honestly the HACK are big enough to really justify new API but that requires as you notice making any module that might use switch regex incompatible :(

Maybe it's possible to propose to PCRE2 devs to introduce some additional API to construct a match_data externally (that is really what we need) but I doubt the would accept something like that. For sure a big reason might be that, that single change permits an almost 1:1 translation of old PCRE API to the new one.

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 this pull request may close these issues.

4 participants