Skip to content
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

dereferencing result of ngx_http_lua_get_req() without checking for NULL #2388

Open
deem0n opened this issue Jan 20, 2025 · 0 comments
Open

Comments

@deem0n
Copy link

deem0n commented Jan 20, 2025

Hello, static checker svacer found several places where we are not checking req object for NULL, while it provided counter-examples where we are doing so.

Problematic files:

r = ngx_http_lua_get_req(L);
if (r != r->main) {
return luaL_error(L, "attempt to read the request body in a "
"subrequest");
}

r = ngx_http_lua_get_req(wait_co_ctx->co);
c = r->connection;

r = ngx_http_lua_get_req(wait_co_ctx->co);
c = r->connection;

r = ngx_http_lua_get_req(L);
if (n == 1 && r == r->main) {
luaL_checktype(L, 1, LUA_TBOOLEAN);
wait = lua_toboolean(L, 1);
}

We have other places where we do proper checks

r = ngx_http_lua_get_req(L);
if (r == NULL) {
return luaL_error(L, "no request found");
}

and 4 more cases in ngx_http_lua_socket_tcp.c, plus one case in ngx_http_lua_util.c:

r = ngx_http_lua_get_req(L);
if (r == NULL) {
return 0;
}

definition of ngx_http_lua_get_req() follows:

static ngx_inline ngx_http_request_t *
ngx_http_lua_get_req(lua_State *L)
{
#ifdef OPENRESTY_LUAJIT
    return lua_getexdata(L);
#else
    ngx_http_request_t    *r;

    lua_getglobal(L, ngx_http_lua_req_key);
    r = lua_touserdata(L, -1);
    lua_pop(L, 1);

    return r;
#endif
}

And it seems, that we don't need to check result of the ngx_http_lua_get_req() as it just returns global object, which is http request object and it must be always available in context of the Lua code execution, because Lua code is always executed inside http request processing pipeline.

Is it right? Can you please confirm that it is false positive static analyzer case ?

If not, I can provide boring MR with trivial fixes.

Thanks!

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

No branches or pull requests

1 participant