Skip to content

Conversation

@xeioex
Copy link
Contributor

@xeioex xeioex commented Jul 1, 2025

This fixes #934 issue on Github.

@xeioex xeioex force-pushed the regexp-square-brackets-escaping branch from 85f7c84 to 2c17999 Compare July 2, 2025 06:48
@xeioex xeioex requested a review from VadimZhestikov July 2, 2025 07:01
@xeioex xeioex marked this pull request as ready for review July 2, 2025 07:01
@VadimZhestikov
Copy link
Contributor

VadimZhestikov commented Jul 2, 2025

It would be nice fix bug in processing regexp /[[]/ as well:

>> /[[]/
Thrown:
SyntaxError: pcre_compile2("[(?!)") failed: missing terminating ] for character class at "" in shell:1
>> /[[^]/
/[[\s\S]/
>>

It could be something like this before your fix:

diff --git a/external/njs_regex.c b/external/njs_regex.c
index a118666b..59e864be 100644
--- a/external/njs_regex.c
+++ b/external/njs_regex.c
@@ -112,18 +112,29 @@ njs_regex_escape(njs_mp_t *mp, njs_str_t *text)
     anychars = 0;
     nomatches = 0;

+    njs_bool_t in;
+
+    in  = 0;
+
     for (p = start; p < end; p++) {
         switch (*p) {
         case '[':
-            if (p + 1 < end && p[1] == ']') {
-                p += 1;
-                nomatches += 1;
-
-            } else if (p + 2 < end && p[1] == '^' && p[2] == ']') {
-                p += 2;
-                anychars += 1;
+            if (!in) {
+                if (p + 1 < end && p[1] == ']') {
+                    p += 1;
+                    nomatches += 1;
+                    break;
+
+                } else if (p + 2 < end && p[1] == '^' && p[2] == ']') {
+                    p += 2;
+                    anychars += 1;
+                    break;
+                }
             }
-
+            in = 1;
+            break;
+        case ']':
+            in = 0;
             break;
         }
     }
@@ -143,20 +154,29 @@ njs_regex_escape(njs_mp_t *mp, njs_str_t *text)

     dst = text->start;

+    in = 0;
+
     for (p = start; p < end; p++) {

         switch (*p) {
         case '[':
-            if (p + 1 < end && p[1] == ']') {
-                p += 1;
-                dst = njs_cpymem(dst, "(?!)", 4);
-                continue;
-
-            } else if (p + 2 < end && p[1] == '^' && p[2] == ']') {
-                p += 2;
-                dst = njs_cpymem(dst, "[\\s\\S]", 6);
-                continue;
+            if (!in) {
+                if (p + 1 < end && p[1] == ']') {
+                    p += 1;
+                    dst = njs_cpymem(dst, "(?!)", 4);
+                    continue;
+
+                } else if (p + 2 < end && p[1] == '^' && p[2] == ']') {
+                    p += 2;
+                    dst = njs_cpymem(dst, "[\\s\\S]", 6);
+                    continue;
+                }
             }
+            in = 1;
+            break;
+        case ']':
+            in = 0;
+            break;
         }

         *dst++ = *p;

xeioex added 2 commits July 2, 2025 12:17
Object leaks:
       ADDRESS REFS SHRF          PROTO CONTENT
0x512000007fc0    1 [module]
nginx: quickjs.c:1967: JS_FreeRuntime: Assertion `list_empty(&rt->gc_obj_list)' failed.

After bellard/quickjs@458c34d2 modules are treated as GC objects and
tracked in rt->gc_obj_list. Intermediary module object loaded in
ngx_qjs_clone() using JS_ReadObject() needed to be freed for proper
ref_count accounting.
@xeioex xeioex force-pushed the regexp-square-brackets-escaping branch from 2c17999 to 29a2423 Compare July 2, 2025 19:26
Copy link
Contributor

@VadimZhestikov VadimZhestikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@xeioex xeioex merged commit 17124c8 into nginx:master Jul 2, 2025
1 check passed
@xeioex xeioex deleted the regexp-square-brackets-escaping branch July 2, 2025 20:01
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.

Failed to run v8-v7 test

2 participants