Skip to content

Commit

Permalink
Fix Race Condition Nginx::Stream::Async.sleep
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
pyama86 committed Nov 8, 2019
1 parent bc2a348 commit 1843183
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 60 deletions.
18 changes: 18 additions & 0 deletions mrbgems/ngx_mruby_mrblib/mrblib/mrb_nginx.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
98 changes: 44 additions & 54 deletions src/stream/ngx_stream_mruby_connection.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand All @@ -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;
}

Expand All @@ -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);

Expand All @@ -125,44 +129,44 @@ 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);
}

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;

Expand All @@ -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);
Expand All @@ -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());
Expand All @@ -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));
}
6 changes: 0 additions & 6 deletions test/t/ngx_mruby.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 1843183

Please sign in to comment.