[njs] Fixed retval handling after an exception.
details: https://hg.nginx.org/njs/rev/585756e8cafe
branches:
changeset: 2337:585756e8cafe
user: Dmitry Volyntsev
date: Wed May 22 23:08:15 2024 -0700
description:
Fixed retval handling after an exception.
Previously, some functions set a retval too early. If this happened
before an exception a partially created object in inconsistent state
may be visible outside the affected functions.
The following functions were fixed:
Object.prototype.valueOf()
Array.prototype.toSpliced()
Array.prototype.toReversed()
Array.prototype.toSorted()
This fixes #713 issue on Github.
diffstat:
src/njs_array.c | 39 ++-
src/njs_object.c | 14 ++
src/test/njs_unit_test.c | 12
3 files changed, 44 insertions(+), 21 deletions(-)
diffs (203 lines):
diff -r 077a5b2f30d8 -r 585756e8cafe src/njs_array.c
--- a/src/njs_array.c Wed May 22 17:26:16 2024 -0700
+++ b/src/njs_array.c Wed May 22 23:08:15 2024 -0700
@@ -591,14 +591,14 @@ njs_array_of(njs_vm_t *vm, njs_value_t *
return NJS_ERROR;
}
-njs_set_array(retval, array);
-
if (array->object.fast_array) {
for (i = 0; i < length; i++) {
array->start[i] = args[i + 1];
}
}
+njs_set_array(retval, array);
+
return NJS_OK;
}
@@ -1388,7 +1388,7 @@ njs_array_prototype_to_spliced(njs_vm_t
{
int64_t i, n, r, start, length, to_insert, to_skip, new_length;
njs_int_tret;
-njs_value_t *this, value;
+njs_value_t *this, a, value;
njs_array_t *array;
this = njs_argument(args, 0);
@@ -1439,7 +1439,7 @@ njs_array_prototype_to_spliced(njs_vm_t
return NJS_ERROR;
}
-njs_set_array(retval, array);
+njs_set_array(, array);
for (i = 0; i < start; i++) {
ret = njs_value_property_i64(vm, this, i, );
@@ -1447,14 +1447,14 @@ njs_array_prototype_to_spliced(njs_vm_t
return NJS_ERROR;
}
-ret = njs_value_create_data_prop_i64(vm, retval, i, , 0);
+ret = njs_value_create_data_prop_i64(vm, , i, , 0);
if (njs_slow_path(ret != NJS_OK)) {
return ret;
}
}
for (n = 3; to_insert-- > 0; i++, n++) {
-ret = njs_value_create_data_prop_i64(vm, retval, i, [n], 0);
+ret = njs_value_create_data_prop_i64(vm, , i, [n], 0);
if (njs_slow_path(ret != NJS_OK)) {
return ret;
}
@@ -1468,7 +1468,7 @@ njs_array_prototype_to_spliced(njs_vm_t
return NJS_ERROR;
}
-ret = njs_value_create_data_prop_i64(vm, retval, i, , 0);
+ret = njs_value_create_data_prop_i64(vm, , i, , 0);
if (njs_slow_path(ret != NJS_OK)) {
return ret;
}
@@ -1477,6 +1477,8 @@ njs_array_prototype_to_spliced(njs_vm_t
i++;
}
+njs_set_array(retval, array);
+
return NJS_OK;
}
@@ -1562,7 +1564,7 @@ njs_array_prototype_to_reversed(njs_vm_t
int64_t length, i;
njs_int_tret;
njs_array_t *array;
-njs_value_t *this, value;
+njs_value_t *this, a, value;
this = njs_argument(args, 0);
@@ -1581,7 +1583,7 @@ njs_array_prototype_to_reversed(njs_vm_t
return NJS_ERROR;
}
-njs_set_array(retval, array);
+njs_set_array(, array);
for (i = 0; i < length; i++) {
ret = njs_value_property_i64(vm, this, length - i - 1, );
@@ -1589,12 +1591,14 @@ njs_array_prototype_to_reversed(njs_vm_t
return NJS_ERROR;
}
-ret = njs_value_create_data_prop_i64(vm, retval, i, , 0);
+ret = njs_value_create_data_prop_i64(vm, , i, , 0);
if (njs_slow_path(ret != NJS_OK)) {
return ret;
}
}
+njs_set_array(retval, array);
+
return NJS_OK;
}
@@ -3018,7 +3022,7 @@ njs_array_prototype_to_sorted(njs_vm_t *
int64_ti, nslots, nunds, length;
njs_int_t ret;
njs_array_t*array;
-njs_value_t*this, *comparefn;
+njs_value_t*this, *comparefn, a;
njs_function_t *compare;
njs_array_sort_slot_t *slots;
@@ -3070,24 +3074,25 @@ njs_array_prototype_to_sorted(njs_vm_t *
njs_assert(length == (nslots + nunds));
-njs_set_array(retval, array);
+njs_set_array(, array);
for (i = 0; i < nslots; i++) {
-ret = njs_value_property_i64_set(vm, retval, i, [i].value);
-if (njs_slow_path(ret == NJS_ERROR)) {
+ret = njs_value_create_data_prop_i64(vm, , i, [i].value, 0);
+if (njs_slow_path(ret != NJS_OK)) {
goto exception;
}
}
for (; i < length; i++) {
-ret = njs_value_property_i64_set(vm, retval, i,
- njs_value_arg(_value_undefined));
-if (njs_slow_path(ret == NJS_ERROR)) {
+ret = njs_value_create_data_prop_i64(vm, , i,
+
Re: I think I found a fix for the memory leak issue on gRPC module
Hi,
On Wed, May 22, 2024 at 06:14:26PM +0400, Roman Arutyunyan wrote:
> Hi,
>
> Indeed there's a problem there. We have similar problems in other places as
> well. Attached is a patch that fixes all I could find.
>
> I did some testing for the sub_filter with the following config. Small
> buffers
> exaggerate the problem.
>
> http {
> server {
> listen 8000;
>
> location / {
> root html;
>
> output_buffers 2 128;
>
> sub_filter 1 2;
> sub_filter_types *;
> sub_filter_once off;
> }
> }
> }
>
> Retrieving a 10m file resulted in 7304 (patched) vs 1317704 (unpatched) bytes
> allocated from the request pool. With the default output_buffers (2 32768),
> it's 79880 vs 84040.
I tested ssi with the following config.
server {
listen 8000;
location / {
root html;
output_buffers 2 128;
ssi on;
ssi_types *;
}
}
The result is similar:
6224 vs 1316912 with small buffers
38864 vs 43952 with the default buffers
> On Thu, May 02, 2024 at 07:59:44AM +, Pavel Pautov via nginx-devel wrote:
> > Hi Sangmin,
> >
> > Your analysis looks correct. Client streaming RPCs can lead to unbound
> > accumulation of ngx_chain_t objects (although, at very slow rate). Gzip
> > module had a similar issue (https://trac.nginx.org/nginx/ticket/1046).
> > Attaching somewhat simplified patch based on yours. I was able to reproduce
> > the issue locally and the patch fixes it.
> >
> > From: nginx-devel On Behalf Of Sangmin Lee
> > Sent: Thursday, April 4, 2024 19:29
> > To: nginx-devel@nginx.org
> > Subject: I think I found a fix for the memory leak issue on gRPC module
> >
> > CAUTION: This email has been sent from an external source. Do not click
> > links, open attachments, or provide sensitive business information unless
> > you can verify the sender's legitimacy.
> >
> > I am sending this mail again because I did a mistake while I was writing a
> > mail. I didn't know, in gmail, "Ctrl - Enter" would send a mail immediately
> > even without a title!
> > I am sorry for that, so I am sending again.
> >
> > Hello,
> >
> > I think I found the main cause of the memory leak issue when using gRPC
> > stream so I made a patch for it.
> > Please find the test scenario and details here -- This is what I wrote.:
> > https://trac.nginx.org/nginx/ticket/2614
> > After I changed the memory pool totally on nginx and tested our workload --
> > long-lived gRPC streams with many connections -- using Valgrind and massif,
> > I was able to find what brought up the memory leak issue.
> > like the picture below.
> >
> > [cid:image001.png@01DA9C29.C792CD90]
> > ( I am not sure whether this picture will be sent properly )
> >
> > After I patched one part, it seems okay now I have tested it for 1 week
> > with out workload.
> >
> > [cid:image002.png@01DA9C29.C792CD90]
> > ( I am not sure whether this picture will be sent properly )
> >
> > But because I am not familiar with Mercurial, I couldn't find a way to
> > create PR like on github. I guess this mailing list is for this patch.
> > From my point of view, it is more like a workaround and I think the way of
> > using ngx_chain_add_copy() or itself needs to be changed because it
> > allocates a ngx_chain_t structure using ngx_alloc_chain_link() but inside
> > of that, it just copies pointer, like cl->buf = in->buf;
> > so this ngx_chain_t instance should be dealt with differently unlike other
> > ngx_chain_t instances.
> > But I am quite new to nginx codes so my view might be wrong.
> > Anyhow, please go over this patch and I would like to further talk here.
> >
> >
> >
> > diff --git a/src/http/modules/ngx_http_grpc_module.c
> > b/src/http/modules/ngx_http_grpc_module.c
> > index dfe49c586..1db67bd0a 100644
> > --- a/src/http/modules/ngx_http_grpc_module.c
> > +++ b/src/http/modules/ngx_http_grpc_module.c
> > @@ -1462,6 +1462,12 @@ ngx_http_grpc_body_output_filter(void *data,
> > ngx_chain_t *in)
> > in = in->next;
> > }
> >
> > + ngx_chain_t *nl;
> > + for (ngx_chain_t *dl = ctx->in; dl != in; dl = nl ) {
> > + nl = dl->next;
> > + ngx_free_chain(r->pool, dl);
> > + }
> > +
> > ctx->in = in;
> >
> > if (last) {
> >
> >
> >
> > Best regards,
> > Sangmin
>
>
>
>
>
> > ___
> > nginx-devel mailing list
> > nginx-devel@nginx.org
> > https://mailman.nginx.org/mailman/listinfo/nginx-devel
>
>
> --
> Roman Arutyunyan
> # HG changeset patch
> # User Roman Arutyunyan
> # Date 1716386385 -14400
> # Wed May 22 17:59:45 2024 +0400
>
