Skip to content
This repository was archived by the owner on Oct 8, 2025. It is now read-only.

Socket: replacement of called functions, permissions, reusability #670

Closed
wants to merge 9 commits into from
Closed

Socket: replacement of called functions, permissions, reusability #670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
31951d1
Socket: fixed parameter definition
echolimazulu Apr 20, 2022
440ecaf
Socket: setting permissions for unix socket
echolimazulu Apr 20, 2022
6e3f50e
Socket: checking socket file type before deleting
echolimazulu Apr 20, 2022
25faad7
Socket: added the ability to get errno for further processing
echolimazulu Apr 20, 2022
2d46b17
Controller: zeroing allocated memory
echolimazulu Apr 20, 2022
491fe57
Socket: transferring error codes to the appropriate file
echolimazulu Apr 20, 2022
2912b54
Socket: preparing to replace connect and rename for listening sockets
echolimazulu Apr 20, 2022
867d4ec
Main: creating a new rcp handler for the main process
echolimazulu Apr 20, 2022
1497d19
Main, Router, Socket: fixed unix socket reuse issue
echolimazulu Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/nxt_conn_connect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ nxt_conn_socket(nxt_task_t *task, nxt_conn_t *c)

family = c->remote->u.sockaddr.sa_family;

s = nxt_socket_create(task, family, c->remote->type, 0, NXT_NONBLOCK);
s = nxt_socket_create(task, family, c->remote->type, 0, NXT_NONBLOCK, NULL);

if (nxt_slow_path(s == -1)) {
return NXT_ERROR;
Expand All @@ -108,7 +108,7 @@ nxt_conn_socket(nxt_task_t *task, nxt_conn_t *c)
c->write_timer.task = task;

if (c->local != NULL) {
if (nxt_slow_path(nxt_socket_bind(task, s, c->local) != NXT_OK)) {
if (nxt_slow_path(nxt_socket_bind(task, s, c->local, NULL) != NXT_OK)) {
nxt_socket_close(task, s);
return NXT_ERROR;
}
Expand Down
2 changes: 1 addition & 1 deletion src/nxt_controller.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ nxt_runtime_controller_socket(nxt_task_t *task, nxt_runtime_t *rt)
{
nxt_listen_socket_t *ls;

ls = nxt_mp_alloc(rt->mem_pool, sizeof(nxt_listen_socket_t));
ls = nxt_mp_zalloc(rt->mem_pool, sizeof(nxt_listen_socket_t));
if (ls == NULL) {
return NXT_ERROR;
}
Expand Down
4 changes: 4 additions & 0 deletions src/nxt_file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ nxt_is_dir(fi) \
nxt_is_file(fi) \
(S_ISREG((fi)->st_mode))

#define \
nxt_is_sock(fi) \
(S_ISSOCK((fi)->st_mode))

#define \
nxt_file_size(fi) \
(fi)->st_size
Expand Down
232 changes: 184 additions & 48 deletions src/nxt_listen_socket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@


static u_char *nxt_listen_socket_log_handler(void *ctx, u_char *pos,
u_char *last);
u_char *end);


nxt_int_t
Expand All @@ -35,11 +35,9 @@ nxt_listen_socket_create(nxt_task_t *task, nxt_mp_t *mp,
nxt_socket_t s;
nxt_thread_t *thr;
nxt_sockaddr_t *sa;
nxt_err_t err;
#if (NXT_HAVE_UNIX_DOMAIN)
int ret;
u_char *p;
nxt_err_t err;
nxt_socket_t ts;
nxt_sockaddr_t *orig_sa;
nxt_file_name_t *name, *tmp;
nxt_file_access_t access;
Expand All @@ -56,12 +54,29 @@ nxt_listen_socket_create(nxt_task_t *task, nxt_mp_t *mp,

family = sa->u.sockaddr.sa_family;

s = nxt_socket_create(task, family, sa->type, 0, ls->flags);
s = nxt_socket_create(task, family, sa->type, 0, ls->flags, &err);
if (s == -1) {

#if (NXT_INET6)

if (err == EAFNOSUPPORT && sa->u.sockaddr.sa_family == AF_INET6) {
ls->error = NXT_SOCKET_ERROR_NOINET6;
}

#endif

if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_socket_create(\"%*s\") failed %E",
(size_t) sa->length, nxt_sockaddr_start(sa), err);
goto fail;
}

if (nxt_socket_setsockopt(task, s, SOL_SOCKET, SO_REUSEADDR, 1) != NXT_OK) {
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_socket_setsockopt(\"%*s\", SO_REUSEADDR) failed",
(size_t) sa->length, nxt_sockaddr_start(sa));
goto fail;
}

Expand All @@ -72,9 +87,14 @@ nxt_listen_socket_create(nxt_task_t *task, nxt_mp_t *mp,

ipv6only = (ls->ipv6only == 1);

/* Ignore possible error. TODO: why? */
(void) nxt_socket_setsockopt(task, s, IPPROTO_IPV6, IPV6_V6ONLY,
ipv6only);
if (nxt_socket_setsockopt(task, s, IPPROTO_IPV6, IPV6_V6ONLY,
ipv6only) != NXT_OK) {
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_socket_setsockopt(\"%*s\", IPV6_V6ONLY) failed",
(size_t) sa->length, nxt_sockaddr_start(sa));
goto fail;
}
}

#endif
Expand All @@ -100,6 +120,10 @@ nxt_listen_socket_create(nxt_task_t *task, nxt_mp_t *mp,

sa = nxt_sockaddr_alloc(mp, sa->socklen + 4, sa->length + 4);
if (sa == NULL) {
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_sockaddr_alloc(%*s) failed",
(size_t) sa->length, nxt_sockaddr_start(sa));
goto fail;
}

Expand All @@ -108,84 +132,135 @@ nxt_listen_socket_create(nxt_task_t *task, nxt_mp_t *mp,

p = nxt_cpystr((u_char *) sa->u.sockaddr_un.sun_path,
(u_char *) orig_sa->u.sockaddr_un.sun_path);
nxt_memcpy(p, ".tmp", 4);
nxt_memcpy(p, NXT_TMP_EXT, 4);

nxt_sockaddr_text(sa);

(void) unlink(sa->u.sockaddr_un.sun_path);
Copy link
Contributor

@hongzhidao hongzhidao Apr 17, 2022

Choose a reason for hiding this comment

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

Well, I think the previous one is good enough.

If you think it's possible that the temp file can be existing like created manually, it happens very rarely.
We can just to use unlink(sa->u.sockaddr_un.sun_path).

My thought is based on we need to do more in the future about this feature to clean up sock files.
So we can just keep the previous code here.

Your way may be used in the future. But now I prefer not to introduce it.

Copy link
Author

@echolimazulu echolimazulu Apr 17, 2022

Choose a reason for hiding this comment

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

This improvement was motivated by this comment:
#649 (comment)

Yes, this function does not implement a check to see if the socket is abandoned or not. But for this, as recommended everywhere and for similar use in linux, a lock is required, which I feel we will not get to soon.

Copy link
Author

@echolimazulu echolimazulu Apr 18, 2022

Choose a reason for hiding this comment

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

In addition to my previous answer, I want to note that ignoring the error handling of (void) unlink here may cause bind to fail later and throw an error (EADDRINUSE) not related to the cause of the failure (e.g. EACCES), but related to the consequence of using bind after (void) unlink.

Copy link
Author

@echolimazulu echolimazulu Apr 18, 2022

Choose a reason for hiding this comment

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

Good morning @mar0x,

Since you suggested this feature earlier, please let me know what you think of my proposed implementation. Thank you!

name = (nxt_file_name_t *) sa->u.sockaddr_un.sun_path;

if (nxt_socket_release_by_path(name) != NXT_OK) {
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_socket_release_by_path(%FN) failed",
name);
goto fail;
}

} else {
orig_sa = NULL;
}

#endif

if (nxt_socket_bind(task, s, sa) != NXT_OK) {
goto fail;
}
if (nxt_socket_bind(task, s, sa, &err) != NXT_OK) {

#if (NXT_HAVE_UNIX_DOMAIN)

if (family == AF_UNIX) {
name = (nxt_file_name_t *) sa->u.sockaddr_un.sun_path;
if (sa->u.sockaddr.sa_family == AF_UNIX) {
switch (err) {

access = (S_IRUSR | S_IWUSR);
case EACCES:
ls->error = NXT_SOCKET_ERROR_ACCESS;
break;

if (nxt_file_set_access(name, access) != NXT_OK) {
goto listen_fail;
}
}
case ENOENT:
case ENOTDIR:
ls->error = NXT_SOCKET_ERROR_PATH;
break;
}

} else
#endif
{
switch (err) {

nxt_debug(task, "listen(%d, %d)", s, ls->backlog);
case EACCES:
ls->error = NXT_SOCKET_ERROR_PORT;
break;

if (listen(s, ls->backlog) != 0) {
nxt_alert(task, "listen(%d, %d) failed %E",
s, ls->backlog, nxt_socket_errno);
goto listen_fail;
}

#if (NXT_HAVE_UNIX_DOMAIN)
case EADDRINUSE:
ls->error = NXT_SOCKET_ERROR_INUSE;
break;

if (orig_sa != NULL) {
ts = nxt_socket_create(task, AF_UNIX, SOCK_STREAM, 0, 0);
if (ts == -1) {
goto listen_fail;
case EADDRNOTAVAIL:
ls->error = NXT_SOCKET_ERROR_NOADDR;
break;
}
}

ret = connect(ts, &orig_sa->u.sockaddr, orig_sa->socklen);
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_socket_bind(%d, %*s) failed %E",
s, (size_t) sa->length, nxt_sockaddr_start(sa),
err);
goto fail;
}

err = nxt_socket_errno;
#if (NXT_HAVE_UNIX_DOMAIN)

nxt_socket_close(task, ts);
if (family == AF_UNIX) {
name = (nxt_file_name_t *) sa->u.sockaddr_un.sun_path;

if (ret == 0) {
nxt_alert(task, "connect(%d, %*s) succeed, address already in use",
ts, (size_t) orig_sa->length,
nxt_sockaddr_start(orig_sa));
access = (ls->access) ? ls->access : (S_IRUSR | S_IWUSR);

if (nxt_file_set_access(name, access) != NXT_OK) {
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_file_set_access(%FN) failed",
name);
goto listen_fail;
}
}

if (err != NXT_ENOENT && err != NXT_ECONNREFUSED) {
nxt_alert(task, "connect(%d, %*s) failed %E",
ts, (size_t) orig_sa->length,
nxt_sockaddr_start(orig_sa), err);
#endif

if (ls->start == NULL) {
Copy link
Author

@echolimazulu echolimazulu Apr 18, 2022

Choose a reason for hiding this comment

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

@hongzhidao How do you like this decision? It is fully consistent with the current logic of the listen and router call program.

Copy link
Contributor

@hongzhidao hongzhidao Apr 18, 2022

Choose a reason for hiding this comment

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

I think the current patch is based on the logic from the controller process, as you used nxt_listen_socket_create(), but it seems it requires main process does listen operation, and now I don't know if it's reasonable.
My suggestion is to make it clear first. Please be patient with the review.

Copy link
Author

@echolimazulu echolimazulu Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The moment I noted, does not perform listen when called nxt_listen_socket_create that came from the router, but at the same time performs it for the controller. That is, this logic corresponds to the already existing logic (without changes) contained in the master branch.

I reproduce new options based on your suggestions and comments as they come in so that we can immediately see the result and move on to the best solution.

Copy link
Contributor

@hongzhidao hongzhidao Apr 19, 2022

Choose a reason for hiding this comment

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

  1. If we perform listen in the main process before all sockets are bound, clients will be able to be accepted but without any process. It's possible to happen in the real world.
  2. If we perform listen in the router process, the logic in the nxt_listen_socket_create is broken, because we don't check the listening connection.
// The below logic is required after `listen`.
#if (NXT_HAVE_UNIX_DOMAIN)

    if (orig_sa != NULL) {
        ts = nxt_socket_create(task, AF_UNIX, SOCK_STREAM, 0, 0);
        if (ts == -1) {
            goto listen_fail;
        }

        ret = connect(ts, &orig_sa->u.sockaddr, orig_sa->socklen);

        err = nxt_socket_errno;

        nxt_socket_close(task, ts);

        if (ret == 0) {
            nxt_alert(task, "connect(%d, %*s) succeed, address already in use",
                      ts, (size_t) orig_sa->length,
                      nxt_sockaddr_start(orig_sa));

            goto listen_fail;
        }

Copy link
Author

@echolimazulu echolimazulu Apr 19, 2022

Choose a reason for hiding this comment

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

I sincerely appreciate your corrections, thank you for this!

You can also move this connect check into a separate function like nxt_listen_socket and call it separately from router and from nxt_listen_socket_create with ls->start == NULL.

Copy link
Contributor

@hongzhidao hongzhidao Apr 21, 2022

Choose a reason for hiding this comment

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

Sorry that personally, I can't agree this new way.

  1. It seems the logic is broken.
static void
nxt_router_conf_error(nxt_task_t *task, nxt_router_temp_conf_t *tmcf)
{
    ...
    for (qlk = nxt_queue_first(&creating_sockets);
         qlk != nxt_queue_tail(&creating_sockets);
         qlk = nxt_queue_next(qlk))
    {
        skcf = nxt_queue_link_data(qlk, nxt_socket_conf_t, link);
        s = skcf->listen->socket;

        if (s != -1) {
            nxt_socket_close(task, s);
        }

        nxt_free(skcf->listen);
    }
  1. It's strange that the router process has to maintain one more socket information about rename. I think it makes things complicated.

We think the ideal solution is to clean up all the unused unix domain socket files.
This is our internal plan for the future.

Copy link
Author

@echolimazulu echolimazulu Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your correction.

I'll fix nxt_router_conf_error, thanks for bringing this to my attention.

But rename cannot be performed directly from router, as it does not have sufficient permissions to perform this operation. This moment is tested by me personally.

The whole difficulty is that you want to ensure similar work of controller and router on the basis of one nxt_listen_socket_create, depending on the place in the code where listen is called, and also provide all the necessary checks and operations after running listen ( connect, rename).

Please tell me more about your ideal solution. I want to note that cleaning up unused (abandoned) sockets and the problem of reusing sockets at the moment are completely different problems.

Copy link
Contributor

@hongzhidao hongzhidao Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please tell me more about your ideal solution. I want to note that cleaning up unused (abandoned) sockets and the problem of reusing sockets at the moment are completely different problems.

If we can clean up unused sockets, then we don't need the way borrowed from controller, though it's more robust. To clean up unused sockets, we need to get the pending deleting sockets in the router process, then ask the controller/main process to do it.

But rename cannot be performed directly from router, as it does not have sufficient permissions to perform this operation. This moment is tested by me personally.

Right, I agree that the way works, but it makes the router more complicated. The router code is already complicated, I prefer not to make it bigger...

Copy link
Author

@echolimazulu echolimazulu Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do not take it for excessive perseverance and obsession. I'm trying to solve a real problem I'm facing in my projects.

The problem is that abandoned and deleted router sockets during a successful (soft) termination may not solve the problem of reusing sockets when the program terminates unexpectedly.

Removing existing sockets must somehow be done during the socket creation process, as it may have been abandoned at the time of the previous unexpected or or early termination of the program.

Previously (#670 (comment)) I suggested not using nxt_listen_socket_create and adding unlink before bind in nxt_main_listening_socket, but you (#670 (comment)) said that it should be abandoned in favor of nxt_listen_socket_create, which involves communication with controller and the need to separate the logic of listen, connect, rename.

When using cloud platforms, the snap-in sends a SIGKILL signal to the container and it is expected that it will complete all the necessary procedures to close all existing processes: connections, sockets, etc. If the end application does not have time to do this, then the container will be forcefully terminated within 10 seconds after SIGKILL.

I understand that in the case of a Unit, 10 seconds is more than enough to terminate all processes, however, but unexpected terminations may not be due to a SIGKILL being passed from a snap.

Copy link
Author

@echolimazulu echolimazulu Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry that personally, I can't agree this new way.

  1. It seems the logic is broken.

Seems to be fixed now: 1497d19

nxt_debug(task, "listen(%d, %d)", s, ls->backlog);

if (listen(s, ls->backlog) != 0) {
err = nxt_socket_errno;
nxt_alert(task, "listen(%d, %d) failed %E",
s, ls->backlog, err);
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"listen(%d, %d) failed %E",
s, ls->backlog, err);
goto listen_fail;
}

tmp = (nxt_file_name_t *) sa->u.sockaddr_un.sun_path;
name = (nxt_file_name_t *) orig_sa->u.sockaddr_un.sun_path;
#if (NXT_HAVE_UNIX_DOMAIN)

if (orig_sa != NULL) {

if (nxt_listen_socket_saddr_check(task, orig_sa) != NXT_OK) {
nxt_alert(task, "nxt_listen_socket_saddr_check(%*s) failed",
(size_t) sa->length, nxt_sockaddr_start(sa));
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_listen_socket_connect(%*s) failed",
(size_t) sa->length, nxt_sockaddr_start(sa));
goto listen_fail;
}

tmp = (nxt_file_name_t *) sa->u.sockaddr_un.sun_path;
name = (nxt_file_name_t *) orig_sa->u.sockaddr_un.sun_path;

if (nxt_file_rename(tmp, name) != NXT_OK) {
nxt_alert(task, "nxt_file_rename(%FN, %FN) failed",
tmp, name);
if (ls->start != NULL && ls->end != NULL)
ls->end = nxt_sprintf(ls->start, ls->end,
"nxt_file_rename(%FN, %FN) failed",
tmp, name);
goto listen_fail;
}

if (nxt_file_rename(tmp, name) != NXT_OK) {
goto listen_fail;
}
}

#endif

}

ls->socket = s;
thr->log = old;

Expand Down Expand Up @@ -215,6 +290,67 @@ nxt_listen_socket_create(nxt_task_t *task, nxt_mp_t *mp,
}


nxt_int_t
nxt_listen_socket_saddr_check(nxt_task_t *task, nxt_sockaddr_t *sa)
{
nxt_socket_t ts;
nxt_int_t ret;
nxt_err_t err;

ts = nxt_socket_create(task, AF_UNIX, SOCK_STREAM, 0, 0, NULL);
if (ts == -1) {
return NXT_ERROR;
}

ret = connect(ts, &sa->u.sockaddr, sa->socklen);
err = nxt_socket_errno;

nxt_socket_close(task, ts);

if (ret == 0) {
nxt_alert(task, "connect(%d, %*s) succeed, address already in use",
ts, (size_t) sa->length,
nxt_sockaddr_start(sa));
return NXT_ERROR;
}

if (err != NXT_ENOENT && err != NXT_ECONNREFUSED) {
nxt_alert(task, "connect(%d, %*s) failed %E",
ts, (size_t) sa->length,
nxt_sockaddr_start(sa), err);
return NXT_ERROR;
}

return NXT_OK;
}


nxt_int_t
nxt_listen_socket_tmp_rename(nxt_task_t *task, nxt_sockaddr_t *sa)
{
size_t TMP_EXT_SIZE = sizeof(NXT_TMP_EXT),
TMP_SIZE = sa->socklen + TMP_EXT_SIZE,
name_size;
nxt_file_name_t tmp[TMP_SIZE], *name;

nxt_memzero(&tmp, TMP_SIZE);

name = (nxt_file_name_t *) sa->u.sockaddr_un.sun_path;
name_size = nxt_strlen(name);

nxt_memcpy(tmp, name, name_size);
nxt_memcpy(tmp + name_size, NXT_TMP_EXT, TMP_EXT_SIZE);

if (nxt_file_rename(tmp, name) != NXT_OK) {
nxt_alert(task, "nxt_file_rename(%FN, %FN) failed",
tmp, name);
return NXT_ERROR;
}

return NXT_OK;
}


nxt_int_t
nxt_listen_socket_update(nxt_task_t *task, nxt_listen_socket_t *ls,
nxt_listen_socket_t *prev)
Expand Down
12 changes: 12 additions & 0 deletions src/nxt_listen_socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
#define _NXT_LISTEN_SOCKET_H_INCLUDED_


#define NXT_TMP_EXT ".tmp"


typedef struct {
/* nxt_socket_t is int. */
nxt_socket_t socket;
Expand All @@ -29,9 +32,16 @@ typedef struct {
#if (NXT_INET6 && defined IPV6_V6ONLY)
uint8_t ipv6only; /* 2 bits */
#endif
#if (NXT_HAVE_UNIX_DOMAIN)
nxt_file_access_t access;
#endif

uint8_t socklen;
uint8_t address_length;

nxt_socket_error_t error;
u_char *start;
u_char *end;
} nxt_listen_socket_t;


Expand Down Expand Up @@ -60,6 +70,8 @@ NXT_EXPORT nxt_int_t nxt_listen_socket_update(nxt_task_t *task,
nxt_listen_socket_t *ls, nxt_listen_socket_t *prev);
NXT_EXPORT void nxt_listen_socket_remote_size(nxt_listen_socket_t *ls);
NXT_EXPORT size_t nxt_listen_socket_pool_min_size(nxt_listen_socket_t *ls);
NXT_EXPORT nxt_int_t nxt_listen_socket_saddr_check(nxt_task_t *task, nxt_sockaddr_t *sa);
NXT_EXPORT nxt_int_t nxt_listen_socket_tmp_rename(nxt_task_t *task, nxt_sockaddr_t *sa);


#endif /* _NXT_LISTEN_SOCKET_H_INCLUDED_ */
Loading