From 1843183da520fdc20e335c7c791579e97a3d83be Mon Sep 17 00:00:00 2001 From: pyama86 Date: Fri, 8 Nov 2019 20:06:14 +0900 Subject: [PATCH] Fix Race Condition Nginx::Stream::Async.sleep Nginx::Stream module is using `mrb->ud`. it is a possibility of becoming a race condition when call Nginx::Stream::Async.sleep. Futhermore, also use `mrb->ud` Nginx::Stream class methods. In this patch, in addition to not using mrb->ud and class methods are treated as warnings. --- mrbgems/ngx_mruby_mrblib/mrblib/mrb_nginx.rb | 18 ++++ src/stream/ngx_stream_mruby_connection.c | 98 +++++++++----------- test/t/ngx_mruby.rb | 6 -- 3 files changed, 62 insertions(+), 60 deletions(-) diff --git a/mrbgems/ngx_mruby_mrblib/mrblib/mrb_nginx.rb b/mrbgems/ngx_mruby_mrblib/mrblib/mrb_nginx.rb index b32b3f8c..6d58a28a 100644 --- a/mrbgems/ngx_mruby_mrblib/mrblib/mrb_nginx.rb +++ b/mrbgems/ngx_mruby_mrblib/mrblib/mrb_nginx.rb @@ -83,6 +83,24 @@ def escape(str) end class Stream + class Connection + %w( + local_ip + local_addr + local_port + local_ip_port + remote_port + remote_ip + remote_addr + proxy_protocol_ip + ).each do |name| + define_singleton_method(name) do + Nginx::Stream.errlogger Nginx::Stream::LOG_WARN, "Nginx::Stream::Connection.#{name} is duplicated. Use instance methods." + Nginx::Stream::Connection.new.send(name) + end + end + end + class Async class << self def sleep(*args) diff --git a/src/stream/ngx_stream_mruby_connection.c b/src/stream/ngx_stream_mruby_connection.c index 058040ab..6136848d 100644 --- a/src/stream/ngx_stream_mruby_connection.c +++ b/src/stream/ngx_stream_mruby_connection.c @@ -19,6 +19,7 @@ typedef struct { ngx_stream_upstream_rr_peer_t *target; ngx_stream_upstream_rr_peers_t *peers; ngx_stream_upstream_srv_conf_t *us; + ngx_stream_mruby_internal_ctx_t *ictx; } ngx_stream_mruby_upstream_context; static void ngx_stream_mrb_upstream_context_free(mrb_state *mrb, void *p) @@ -39,10 +40,8 @@ static mrb_value ngx_stream_mrb_connection_init(mrb_state *mrb, mrb_value self) ngx_stream_mruby_upstream_context *ctx; ngx_stream_upstream_main_conf_t *umcf; ngx_stream_upstream_srv_conf_t **usp; - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; - mrb_get_args(mrb, "o", &upstream); + mrb_get_args(mrb, "|o", &upstream); ctx = (ngx_stream_mruby_upstream_context *)DATA_PTR(self); if (ctx) { @@ -52,36 +51,42 @@ static mrb_value ngx_stream_mrb_connection_init(mrb_state *mrb, mrb_value self) ctx = (ngx_stream_mruby_upstream_context *)mrb_malloc(mrb, sizeof(ngx_stream_mruby_upstream_context)); - ctx->upstream = upstream; ctx->target = NULL; ctx->peers = NULL; ctx->us = NULL; + ctx->ictx = (ngx_stream_mruby_internal_ctx_t *)mrb_malloc(mrb, sizeof(ngx_stream_mruby_internal_ctx_t)); + ctx->ictx->s = (ngx_stream_session_t *)mrb_malloc(mrb, sizeof(ngx_stream_session_t)); + + *ctx->ictx->s = *((ngx_stream_mruby_internal_ctx_t *)mrb->ud)->s; + ctx->ictx->stream_status = ((ngx_stream_mruby_internal_ctx_t *)mrb->ud)->stream_status; + ngx_stream_session_t *s = ctx->ictx->s; + umcf = ngx_stream_get_module_main_conf(s, ngx_stream_upstream_module); usp = umcf->upstreams.elts; - for (i = 0; i < umcf->upstreams.nelts; i++) { - if (ngx_strncasecmp(usp[i]->host.data, (u_char *)RSTRING_PTR(upstream), RSTRING_LEN(upstream)) == 0) { - ctx->us = usp[i]; - ctx->peers = usp[i]->peer.data; - if (ctx->peers->number > 1) { - mrb_raise(mrb, E_RUNTIME_ERROR, "don't support multiple server config"); + if (!mrb_nil_p(upstream)) { + ctx->upstream = upstream; + for (i = 0; i < umcf->upstreams.nelts; i++) { + if (ngx_strncasecmp(usp[i]->host.data, (u_char *)RSTRING_PTR(upstream), RSTRING_LEN(upstream)) == 0) { + ctx->us = usp[i]; + ctx->peers = usp[i]->peer.data; + if (ctx->peers->number > 1) { + mrb_raise(mrb, E_RUNTIME_ERROR, "don't support multiple server config"); + } + ctx->target = ctx->peers->peer; + break; } - ctx->target = ctx->peers->peer; - break; + } + if (ctx->us == NULL || ctx->peers == NULL) { + mrb_raisef(mrb, E_RUNTIME_ERROR, "%S not found upstream config", upstream); + } + if (ctx->target == NULL) { + mrb_raise(mrb, E_RUNTIME_ERROR, "not found server config in upstream"); } } - mrb_data_init(self, ctx, &ngx_stream_mrb_upstream_context_type); - if (ctx->us == NULL || ctx->peers == NULL) { - mrb_raisef(mrb, E_RUNTIME_ERROR, "%S not found upstream config", upstream); - } - - if (ctx->target == NULL) { - mrb_raise(mrb, E_RUNTIME_ERROR, "not found server config in upstream"); - } - return self; } @@ -99,8 +104,7 @@ static mrb_value ngx_stream_mrb_upstream_set_server(mrb_state *mrb, mrb_value se ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); ngx_url_t u; mrb_value server; - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; + ngx_stream_session_t *s = ctx->ictx->s; mrb_get_args(mrb, "o", &server); @@ -125,35 +129,35 @@ static mrb_value ngx_stream_mrb_upstream_set_server(mrb_state *mrb, mrb_value se static mrb_value ngx_stream_mrb_connection_get_status(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); - return mrb_fixnum_value((mrb_int)ictx->stream_status); + return mrb_fixnum_value((mrb_int)ctx->ictx->stream_status); } static mrb_value ngx_stream_mrb_connection_status(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); mrb_int status; mrb_get_args(mrb, "i", &status); - ictx->stream_status = (ngx_int_t)status; + ctx->ictx->stream_status = (ngx_int_t)status; return self; } static mrb_value ngx_stream_mrb_remote_ip(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); + ngx_stream_session_t *s = ctx->ictx->s; return mrb_str_new(mrb, (const char *)s->connection->addr_text.data, s->connection->addr_text.len); } static mrb_value ngx_stream_mrb_local_ip_port(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); + ngx_stream_session_t *s = ctx->ictx->s; return mrb_str_new(mrb, (const char *)s->connection->listening->addr_text.data, s->connection->listening->addr_text.len); @@ -161,8 +165,8 @@ static mrb_value ngx_stream_mrb_local_ip_port(mrb_state *mrb, mrb_value self) static mrb_value ngx_stream_mrb_local_ip(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); + ngx_stream_session_t *s = ctx->ictx->s; u_char ipaddr_txt[NGX_SOCKADDR_STRLEN]; mrb_int ipaddr_len; @@ -183,24 +187,24 @@ static in_port_t get_in_port(struct sockaddr *sa) static mrb_value ngx_stream_mrb_local_port(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); + ngx_stream_session_t *s = ctx->ictx->s; return mrb_fixnum_value(ntohs(get_in_port(s->connection->local_sockaddr))); } static mrb_value ngx_stream_mrb_remote_port(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); + ngx_stream_session_t *s = ctx->ictx->s; return mrb_fixnum_value(ntohs(get_in_port(s->connection->sockaddr))); } static mrb_value ngx_stream_mrb_proxy_protocol_addr(mrb_state *mrb, mrb_value self) { - ngx_stream_mruby_internal_ctx_t *ictx = mrb->ud; - ngx_stream_session_t *s = ictx->s; + ngx_stream_mruby_upstream_context *ctx = DATA_PTR(self); + ngx_stream_session_t *s = ctx->ictx->s; return mrb_str_new(mrb, (const char *)s->connection->proxy_protocol_addr.data, s->connection->proxy_protocol_addr.len); @@ -212,7 +216,7 @@ void ngx_stream_mrb_conn_class_init(mrb_state *mrb, struct RClass *class) class_conn = mrb_define_class_under(mrb, class, "Connection", mrb->object_class); MRB_SET_INSTANCE_TT(class_conn, MRB_TT_DATA); - mrb_define_method(mrb, class_conn, "initialize", ngx_stream_mrb_connection_init, MRB_ARGS_REQ(1)); + mrb_define_method(mrb, class_conn, "initialize", ngx_stream_mrb_connection_init, MRB_ARGS_OPT(1)); mrb_define_method(mrb, class_conn, "upstream_server", ngx_stream_mrb_upstream_get_server, MRB_ARGS_NONE()); mrb_define_method(mrb, class_conn, "upstream_server=", ngx_stream_mrb_upstream_set_server, MRB_ARGS_REQ(1)); mrb_define_method(mrb, class_conn, "stream_status", ngx_stream_mrb_connection_get_status, MRB_ARGS_NONE()); @@ -229,18 +233,4 @@ void ngx_stream_mrb_conn_class_init(mrb_state *mrb, struct RClass *class) mrb_define_method(mrb, class_conn, "proxy_protocol_ip", ngx_stream_mrb_proxy_protocol_addr, MRB_ARGS_NONE()); mrb_define_method(mrb, class_conn, "proxy_protocol_addr", ngx_stream_mrb_proxy_protocol_addr, MRB_ARGS_NONE()); - - mrb_define_class_method(mrb, class_conn, "local_ip", ngx_stream_mrb_local_ip, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "local_addr", ngx_stream_mrb_local_ip, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "local_port", ngx_stream_mrb_local_port, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "local_ip_port", ngx_stream_mrb_local_ip_port, MRB_ARGS_NONE()); - - mrb_define_class_method(mrb, class_conn, "remote_port", ngx_stream_mrb_remote_port, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "remote_ip", ngx_stream_mrb_remote_ip, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "remote_addr", ngx_stream_mrb_remote_ip, MRB_ARGS_NONE()); - - mrb_define_class_method(mrb, class_conn, "proxy_protocol_ip", ngx_stream_mrb_proxy_protocol_addr, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "proxy_protocol_addr", ngx_stream_mrb_proxy_protocol_addr, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "stream_status", ngx_stream_mrb_connection_get_status, MRB_ARGS_NONE()); - mrb_define_class_method(mrb, class_conn, "stream_status=", ngx_stream_mrb_connection_status, MRB_ARGS_REQ(1)); } diff --git a/test/t/ngx_mruby.rb b/test/t/ngx_mruby.rb index a4c7cb6f..df6f5bed 100644 --- a/test/t/ngx_mruby.rb +++ b/test/t/ngx_mruby.rb @@ -641,15 +641,9 @@ def is_async_supported? if nginx_features.is_async_supported? t.assert('ngx_mruby - Nginx.Async.sleep', 'location /async_sleep') do - t = Thread.new do - res = HttpRequest.new.get base + '/async_sleep' - t.assert_equal 'body', res["body"] - t.assert_equal 200, res.code - end res = HttpRequest.new.get base + '/async_sleep' t.assert_equal 'body', res["body"] t.assert_equal 200, res.code - t.join end t.assert('ngx_mruby - Nginx.Async.sleep looping', 'location /async_sleep_loop') do