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

pcre #72

Closed
Vittly opened this Issue Nov 26, 2011 · 25 comments

Comments

Projects
None yet
3 participants
@Vittly

Vittly commented Nov 26, 2011

Hi again! As I said earlier I am using pcre 8.2 with 'j' and 'u' flags in ngx.match. From time to time I got 'pcre_compile() failed to get memory...' is it bug of lua_nginx or nginx or pcre? What should I do? Do not use pcre?

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 26, 2011

On Sun, Nov 27, 2011 at 12:43 AM, Vittly
reply@reply.github.com
wrote:

Hi again! As I said earlier I am using pcre 8.2 with 'j' and 'u' flags in ngx.match. From time to time I got 'pcre_compile() failed to get memory...' is it bug of lua_nginx or nginx or pcre? What should I do? Do not use pcre?

Could you please give me a minimized sample that can reproduce this
problem? It seems like a bug in ngx_lua.

Thanks!
-agentzh

@Vittly

This comment has been minimized.

Vittly commented Nov 27, 2011

Seems that it is a memory leak because application could work properly all day and in the end of day I see that all regexs crashes. I think it is important that all regexs I use contains captures '(...)'. To reproduce the error try thomething like
location / {
ngx.re.match(ngx.req.get_headers()['User-Agent'],'(safari/(\d+).(\d+)'))
}
and call it in script 1000 times or more. I used few pcre versions and different match flags and all the time error was the same.

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 27, 2011

On Sun, Nov 27, 2011 at 1:26 PM, Vittly
reply@reply.github.com
wrote:

Seems that it is a memory leak because application could work properly all day and in the end of day I see that all regexs crashes. I think it is important that all regexs I use contains captures '(...)'. To reproduce the error try thomething like
location / {
 ngx.re.match(ngx.req.get_headers()['User-Agent'],'(safari/(\d+).(\d+)'))
}
and call it in script 1000 times or more. I used few pcre versions and different match flags and all the time error was the same.

Thanks for the report! But I still need further information:

  1. The output of the "nginx -V" command
  2. The output of the "uname -a" command

Also, could you try adding the "o" regex option to your regex? As in

location / {

  content_by_lua "
ngx.re.match(ngx.req.get_headers()['User-Agent'],'(safari/(\d+).(\d+))',
'o')
";
}

Thanks!
-agentzh

@Vittly

This comment has been minimized.

Vittly commented Nov 27, 2011

Ok.
nginx -V
nginx: nginx version: nginx/1.0.10
nginx: built by gcc 4.4.5 (Debian 4.4.5-8)
nginx: TLS SNI support enabled
nginx: configure arguments: --prefix=/opt/nginx --add-module=../ngx_devel_kit --add-module=../chaoslawful --with-pcre=../pcre-8.2 --with-http_ssl_module

uname -a
Linux ip---- 2.6.32-5-xen-amd64 #1 SMP Mon Oct 3 07:53:54 UTC 2011 x86_64 GNU/Linux

would 'o' option work with 'j'? Or I need to replace 'j' by 'o' for a while?

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 27, 2011

On Sun, Nov 27, 2011 at 1:49 PM, Vittly
reply@reply.github.com
wrote:

Ok.
nginx -V
nginx: nginx version: nginx/1.0.10
nginx: built by gcc 4.4.5 (Debian 4.4.5-8)
nginx: TLS SNI support enabled
nginx: configure arguments: --prefix=/opt/nginx --add-module=../ngx_devel_kit --add-module=../chaoslawful --with-pcre=../pcre-8.2 --with-http_ssl_module

"pcre 8.2" is actually referring to pcre 8.20 here, right?

Which version of ngx_lua are you using? v0.3.1rc34?

uname -a
Linux ip---- 2.6.32-5-xen-amd64 #1 SMP Mon Oct 3 07:53:54 UTC 2011 x86_64 GNU/Linux

would 'o' option work with 'j'? Or I need to replace 'j' by 'o' for a while?

The "j" option should always be used with "o", or the regex will be
compiled and JIT'd at every run and the benefit of JITing is going
away. Maybe I should have made this clear in the docs of ngx_lua :)

Thanks!
-agentzh

@Vittly

This comment has been minimized.

Vittly commented Nov 27, 2011

"pcre 8.2" is actually referring to pcre 8.20 here, right? Yes!
Which version of ngx_lua are you using? v0.3.1rc34? Yes!

ok I ll add 'o' to 'j'

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 27, 2011

On Sun, Nov 27, 2011 at 1:26 PM, Vittly
reply@reply.github.com
wrote:

Seems that it is a memory leak because application could work properly all day and in the end of day I see that all regexs crashes. I think it is important that all regexs I use contains captures '(...)'. To reproduce the error try thomething like
location / {
 ngx.re.match(ngx.req.get_headers()['User-Agent'],'(safari/(\d+).(\d+)'))
}
and call it in script 1000 times or more. I used few pcre versions and different match flags and all the time error was the same.

I've tried a slightly modified version of your example which is buggy
in some minor places:

location /re {
    content_by_lua "
        ngx.req.set_header('User-Agent', 'safari')
        ngx.re.match(ngx.req.get_headers()['User-Agent'],[[(safari/(\\d+)\\.(\\d+))]])
        ngx.say('done')
    ";
}

with ngx_lua v0.3.1rc36 and nginx 1.0.10 and pcre 8.20 (with and
without --enable-jit for my pcre build) on Linux x86_64, similar to
yours. When hitting this location with ab for tens of thousands of
requests, there's no error messages at all in my error.log and no
memory leak observed in my "top" utility.

Could you please try out my example as is on your side?

It'll be easier for me to fix this issue if you can help me reproduce
it on my side ;)

BTW, pcre jit is disabled by default in pcre 8.20's build system. You
need to pass --enable-jit to it, maybe something like this for your
nginx ./configure command:

./configure --with-pcre=/path/to/your/pcre/source/tree

--with-pcre-opt="--enable-jit" ...

You should pass --enable-utf8 too if you want to enable the UTF-8
support in your PCRE build at the same time :)

Thanks!
-agentzh

@Vittly

This comment has been minimized.

Vittly commented Nov 27, 2011

I know about this flags and it wasn't simple task. For example your method " ./configure --with-pcre=/path/to/your/pcre/source/tree
--with-pcre-opt="--enable-jit"" is not working becouse with-pcre-opt is list of options for make not configure. To put '--enable' right in nginx building process you should ./configure nginx and then modify objs/Makefile as shown at http://www.cslog.cn/Content/nginx-pcre-utf8-rewrite/ (there are utf8 and unicode, jit could be placed there too). Mey be you should write this technique in your documentation it helps people do not waste time.
Ok? I'll put your code in my application but how could it help you if error occurs? How would we locate it?

@Vittly

This comment has been minimized.

Vittly commented Nov 27, 2011

You also could add to your documentation that r34 installs only if $CFLAGS = -Wno-error (in nginx makefile). Pleeeease:)

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 28, 2011

On Sun, Nov 27, 2011 at 11:15 PM, Vittly
reply@reply.github.com
wrote:

You also could add to your documentation that r34 installs only id $CFLAGS = -Wno-error (in nginx makefile). Pleeeease:)

Can you try the latest rc36 release? I think I've fixed this issue in
rc34 already.

-agentzh

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 28, 2011

On Sun, Nov 27, 2011 at 11:01 PM, Vittly
reply@reply.github.com
wrote:

I know about this flags and it wasn't simple task. For example your method " ./configure --with-pcre=/path/to/your/pcre/source/tree
--with-pcre-opt="--enable-jit"" is not working becouse with-pcre-opt is list of options for make not configure. To put '--enable' right in nginx building process you should ./configure nginx and then modify objs/Makefile as shown at http://www.cslog.cn/Content/nginx-pcre-utf8-rewrite/ (there are utf8 and unicode, jit could be placed there too). Mey be you should write this technique in your documentation it helps people do not waste time.

This is unfortunate. Then I'd recommend building pcre separately and
make nginx use it via the --with-cc-opt and --with-ld-opt configure
options. Here's an example:

./configure \
     --with-cc-opt=$'-I/opt/pcre820jit/include' \
     --with-ld-opt="-L/opt/pcre820jit/lib -Wl,-rpath,/opt/pcre820jit/lib \
     ...

assuming that your pcre is installed into /opt/pcre820jit as I am ;)
Here's the command I used to build and install pcre:

./configure --prefix=/opt/pcre820jit --enable-jit --enable-utf8
make -j2
sudo make install

Ok? I'll put your code in my application but how could it help you if error occurs? How would we locate it?

This is just a sample meant to reproduce the issue that you have. It's
not meant to be integrated into your application. I hope that you can
use a separate nginx.conf to include this location definition and run
it to try to reproduce the issue on your side.

Best,
-agentzh

@Vittly

This comment has been minimized.

Vittly commented Nov 28, 2011

I have done 1 000 000 requests for ngx.re.match(ngx.req.get_headers()['User-Agent'],...) but it does not go down. Then I ve told my friends (which cause the errors before) to request this "regex" location but during the day there was not any errors at all. Main application logic now use 'jo' flags and similary do not crash.

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 28, 2011

On Mon, Nov 28, 2011 at 12:15 PM, Vittly
reply@reply.github.com
wrote:

I have done 1 000 000 requests for ngx.re.match(ngx.req.get_headers()['User-Agent'],...) but it does not go down. Then I ve told my friends (which cause the errors before) to request this "regex" location but during the day there was not any errors at all. Main application logic now use 'jo' flags and similary do not crash.

Good to know :) But I still want to fix the case that "o" is not
specified. Your earlier standalone sample dos not lead to any errors
on my side. Can you paste the exact nginx.conf that you can produce
this issue. Your earlier snippet does have syntax errors:

location / {
    ngx.re.match(ngx.req.get_headers()['User-Agent'],'(safari/(\\d+)\\.(\\d+)'))
}

You definitely lost content_by_lua directive and the parentheses did
not match at all.

Please send me the exact config snippet that you actually run and that
can reproduce the "failed to allocate memory" error on your side.

Thanks!
-agentzh

@eoranged

This comment has been minimized.

eoranged commented Nov 28, 2011

I've faced the issue during playing with shared dictionaries. I have only few regex'es (something about 3 or 4) in my code, all with and only with "o" option.
The error is happend just once and I've failed reproducing It, so I've decided that it was my fault.

@Vittly

This comment has been minimized.

Vittly commented Nov 29, 2011

Yes, I also could not reproduce the error:(

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 29, 2011

On Tue, Nov 29, 2011 at 11:52 AM, Vittly
reply@reply.github.com
wrote:

Yes, I also could not reproduce the error:(

Okay, I've finally designed a use case that can reproduce this error:

location /re {
    content_by_lua '
        ngx.re.match("hello", "hello", "j")
        ngx.say("done")
    ';
    header_filter_by_lua '
        ngx.re.match("hello", "world", "j")
    ';
}

Then GET /re will yield the following error message in my error.log:

pcre_compile() failed: failed to get memory in ...

Another use case can also lead to similar issues is as follows:

location /re {
    content_by_lua '
        ngx.re.match("hello", "hello", "j")
        ngx.exec("/foo")
    ';
}

location /foo {
    internal;
    content_by_lua '
        ngx.re.match("hello", "world", "j")
        ngx.say("done")
    ';
}

I've already fixed this pcre malloc/free issues in my local copy of
ngx_lua. I'll commit my fix once I've run all the tests for ngx_lua on
my local tree :)

Thanks for reporting this issue!
-agentzh

agentzh added a commit that referenced this issue Nov 29, 2011

bugfix: use of the ngx.re API might lead to errors like "pcre_compile…
…() failed: failed to get memory" due to incorrect pcre_malloc and pcre_free handling. thanks Vittly for reporting this in github issue #72.
@Vittly

This comment has been minimized.

Vittly commented Nov 29, 2011

Good! Let me know when next version would finished, I ll try it and close the issue

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 29, 2011

Please try out the v0.3.1rc37 release hopefully this issue is fixed completely now :)

https://github.com/chaoslawful/lua-nginx-module/tags

Thanks!

@eoranged

This comment has been minimized.

eoranged commented Nov 29, 2011

@agentzh Is it possible that the bug will occur in expressions like ngx.re.match('1234-1234', '(\d{4})(\d{4})', 'o')?
I haven't j option in my ngx.re.match, but I've faced the issue week or maybe two weeks ago.
It happend just once and I wasn't able to reproduce it, but we didn't tested this in production, so I'm afraid a bit.

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 30, 2011

On Wed, Nov 30, 2011 at 4:08 AM, Vladimir Protasov
reply@reply.github.com
wrote:

@agentzh Is it possible that the bug will occur in expressions like ngx.re.match('1234-1234', '(\d{4})(\d{4})', 'o')?

I don't think so :) But you may have issues when signal the nginx to
reload the config with HUP.

I haven't j option in my ngx.re.match, but I've faced the issue week or maybe two weeks ago.
It happend just once and I wasn't able to reproduce it, but we didn't tested this in production, so I'm afraid a bit.

Please consider upgrading ngx_lua (or ngx_openresty in general) to the
latest version. The old approach of handling pcre memory is kinda
broken. And your feedback on the new version (either with "j" or "o"
or "jo" or other regex option combinations) is highly appreciated as
usual ;)

Thanks!
-agentzh

@eoranged

This comment has been minimized.

eoranged commented Nov 30, 2011

@agentzh I'm always on the latest version and I'll try to provide feedback.
Which problems may appear due to reloading config file? Our development server configuration/scripts are updated few times everyday, so It's possible that HUPing the server is the reason.

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 30, 2011

On Wed, Nov 30, 2011 at 7:50 PM, Vladimir Protasov
reply@reply.github.com
wrote:

@agentzh I'm always on the latest version and I'll try to provide feedback.
Which problems may appear due to reloading config file?

The latest version (v0.3.1rc37) of ngx_lua should not have this issue.
Older versions might produce the "pcre_compile() failed to get
memory..." error in your error.log. But if the latest version still
give this error or any other pcre related error messages, please let
me know :)

Our development server configuration/scripts are updated few times everyday, so It's possible that HUPing the server is the reason.

Fair enough :)

Regards,
-agentzh

@eoranged

This comment has been minimized.

eoranged commented Nov 30, 2011

30.11.2011, 16:01, "agentzh (章亦春)" reply@reply.github.com:

On Wed, Nov 30, 2011 at 7:50 PM, Vladimir Protasov
reply@reply.github.com
wrote:

 @agentzh I'm always on the latest version and I'll try to provide feedback.
 Which problems may appear due to reloading config file?

The latest version (v0.3.1rc37) of ngx_lua should not have this issue.
Older versions might produce the "pcre_compile() failed to get
memory..." error in your error.log. But if the latest version still
give this error or any other pcre related error messages, please let
me know :)

I've seen the message only once and If it will appear again, I will report about.
Thanks a lot.

 Our development server configuration/scripts are updated few times everyday, so It's possible that HUPing the server is the reason.

Fair enough :)

Regards,
-agentzh


Reply to this email directly or view it on GitHub:
#72 (comment)

Best regards,
Vladimir Protasov.

@agentzh

This comment has been minimized.

Member

agentzh commented Apr 19, 2012

Any update on this issue?

@Vittly

This comment has been minimized.

Vittly commented Apr 19, 2012

I have been using ngx.re with 'jo' few month and all is ok. But I don't know anything about its status without these options. I am happy with 'jo' so I am closing the issue

@Vittly Vittly closed this Apr 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment