You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/06/10 14:02:33 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #3895: net/accept: alloc the accept fd after accept success
anchao opened a new pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895
## Summary
net/accept: alloc the accept fd after accept success
The new fd will be in a pending state before accept is successful, if the application creates a task when accepting, the pending fd will be dup(), but since newsock->s_conn has not been initialized, assert will be triggered when inet_addref()
`[ 314.683402] [76] [ EMERG] up_assert: Assertion failed at file:inet/inet_sockif.c line: 312 task: pt-0xc19efad`
https://github.com/apache/incubator-nuttx/blob/7c20199a61f61f1bd781cebee34cdc2547841564/net/inet/inet_sockif.c#L312
## Impact
N/A
## Testing
Thread 1 : accept()
Thread 2 : posix_spawn()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-858783554
It's a common case that the main thread is blocking in accept to wait the new connection, and many work threads service the client request concurrently. If the work thread decide to execute some program, the crash will happen immediately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#discussion_r649718984
##########
File path: fs/socket/socket.c
##########
@@ -285,6 +284,9 @@ int socket(int domain, int type, int protocol)
errout_with_sockfd:
nx_close(sockfd);
Review comment:
Done
##########
File path: net/socket/accept.c
##########
@@ -259,29 +260,36 @@ int accept(int sockfd, FAR struct sockaddr *addr, FAR socklen_t *addrlen)
goto errout;
}
- /* Allocate a socket descriptor for the new connection now (so that it
- * cannot fail later)
- */
-
- newfd = sockfd_allocate(&newsock, O_RDWR);
- if (newfd < 0)
+ newsock = kmm_zalloc(sizeof(*newsock));
+ if (newsock == NULL)
{
- errcode = ENFILE;
+ errcode = ENOMEM;
goto errout;
}
ret = psock_accept(psock, addr, addrlen, newsock);
if (ret < 0)
{
errcode = -ret;
- goto errout_with_socket;
+ goto errout_with_alloc;
+ }
+
+ /* Allocate a socket descriptor for the new connection now (so that it
+ * cannot fail later)
+ */
+
+ newfd = sockfd_allocate(newsock, O_RDWR);
+ if (newfd < 0)
+ {
+ errcode = ENFILE;
+ goto errout_with_alloc;
}
leave_cancellation_point();
return newfd;
-errout_with_socket:
- nx_close(newfd);
+errout_with_alloc:
+ kmm_free(newsock);
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#discussion_r649653564
##########
File path: net/socket/accept.c
##########
@@ -259,29 +263,31 @@ int accept(int sockfd, FAR struct sockaddr *addr, FAR socklen_t *addrlen)
goto errout;
}
+ ret = psock_accept(psock, addr, addrlen, &newsock);
Review comment:
Good, catch.
##########
File path: net/socket/accept.c
##########
@@ -259,29 +263,31 @@ int accept(int sockfd, FAR struct sockaddr *addr, FAR socklen_t *addrlen)
goto errout;
}
+ ret = psock_accept(psock, addr, addrlen, &newsock);
Review comment:
Good catch, need more investigate here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-859222599
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-859258352
> > > i think even socket() has the same problem potentially.
> > > maybe it's cleaner to separate sockfd_allocate into two.
> > >
> > > * allocate socket (just kmm_zalloc at this point)
> > > * allocate a file and associate the given already initialized socket
> >
> >
> > Yes, there is potential risk in other case, @Donny9 please find a general solution to fix the race condition. But this patch is valuable byself to avoid waste one additional file handle resource. @yamt do you think so?
>
> i'm not against a quick fix as far as it doesn't have other issues. (like tcp_start_monitor thing in this patch)
Hi @yamt san,
In the new rebase, I moved the psock alloc outside of sockfd_allocate, please review it again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#discussion_r649607634
##########
File path: net/socket/accept.c
##########
@@ -259,29 +263,31 @@ int accept(int sockfd, FAR struct sockaddr *addr, FAR socklen_t *addrlen)
goto errout;
}
+ ret = psock_accept(psock, addr, addrlen, &newsock);
Review comment:
this doesn't look safe as accept can record the reference to newsock somewhere.
eg. tcp_start_monitor
##########
File path: net/socket/accept.c
##########
@@ -259,29 +260,36 @@ int accept(int sockfd, FAR struct sockaddr *addr, FAR socklen_t *addrlen)
goto errout;
}
- /* Allocate a socket descriptor for the new connection now (so that it
- * cannot fail later)
- */
-
- newfd = sockfd_allocate(&newsock, O_RDWR);
- if (newfd < 0)
+ newsock = kmm_zalloc(sizeof(*newsock));
+ if (newsock == NULL)
{
- errcode = ENFILE;
+ errcode = ENOMEM;
goto errout;
}
ret = psock_accept(psock, addr, addrlen, newsock);
if (ret < 0)
{
errcode = -ret;
- goto errout_with_socket;
+ goto errout_with_alloc;
+ }
+
+ /* Allocate a socket descriptor for the new connection now (so that it
+ * cannot fail later)
+ */
+
+ newfd = sockfd_allocate(newsock, O_RDWR);
+ if (newfd < 0)
+ {
+ errcode = ENFILE;
+ goto errout_with_alloc;
}
leave_cancellation_point();
return newfd;
-errout_with_socket:
- nx_close(newfd);
+errout_with_alloc:
+ kmm_free(newsock);
Review comment:
i suspect you should use psock_close here.
##########
File path: fs/socket/socket.c
##########
@@ -285,6 +284,9 @@ int socket(int domain, int type, int protocol)
errout_with_sockfd:
nx_close(sockfd);
Review comment:
doesn't this free the psock and causes double free below?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-858783554
It's a common case that the main thread is blocking in accept to wait the new connection, and many work threads service the client request concurrently. If a work thread decides to execute some program, the crash will happen immediately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-859222599
> i think even socket() has the same problem potentially.
>
> maybe it's cleaner to separate sockfd_allocate into two.
>
> * allocate socket (just kmm_zalloc at this point)
> * allocate a file and associate the given already initialized socket
Yes, there is potential risk in other case, @Donny9 please find a general solution to fix the race condition. But this patch is valuable byself to avoid waste one additional file handle resource. @yamt do you think so?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-858740954
> The new fd will be in a pending state before accept is successful, if the application creates a task when accepting, the pending fd will be dup(), but since newsock->s_conn has not been initialized, assert will be triggered when inet_addref()
do you mean that, while a thread is blocking in accept(), another thread in the same task group causes dup() of the fd?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-859176868
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #3895: net/accept: alloc the accept fd after accept success
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3895:
URL: https://github.com/apache/incubator-nuttx/pull/3895#issuecomment-858771262
> > The new fd will be in a pending state before accept is successful, if the application creates a task when accepting, the pending fd will be dup(), but since newsock->s_conn has not been initialized, assert will be triggered when inet_addref()
>
> do you mean that, while a thread is blocking in accept(), another thread in the same task group causes dup() of the fd?
Yep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org