You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/09/09 14:07:05 UTC
[GitHub] [thrift] emmenlau opened a new pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
emmenlau opened a new pull request #2234:
URL: https://github.com/apache/thrift/pull/2234
The code that sets a domain socket path in `TServerSocket.cpp` and `TSocket.cpp` is currently slightly more complex than required (at least that is my understanding). Also it seems to contain a potential out-of-bounds-access.
I simplified the code in the following way:
- Ensure that the `sockaddr_un struct` is fully set to zero before using it
- This removes the necessity to deal with zero-termination of the path. It also removes the need for the `len` variable
- Do not use `path_.size() + 1` length when `memcpy` the path from `path_` to the `sockaddr_un struct`
- This may be an out-of-bounds-access in the current code, depending on whether `path_` actually contains an additional zero-terminating character (which is unlikely)
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [x] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
----------------------------------------------------------------
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] [thrift] deiv commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
deiv commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485670897
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
Review comment:
I think, this will break the abstract unix socket type name. You need the length of the string without the null termination for abstract unix sockets. For pathname unix sockets, the null termination is needed.
----------------------------------------------------------------
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] [thrift] ulidtko commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
ulidtko commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485650407
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(address)))) {
Review comment:
I'm a bit wary about this `sizeof(address)` — because of how `struct sockaddr` can have more data following it. What type is `address` again?..
`man 7 unix` says this:
> **Pathname sockets**
> When binding a socket to a pathname, a few rules should be observed for maximum portability and ease of coding:
>
> * The pathname in sun_path should be null-terminated.
> * The length of the pathname, including the terminating null byte, should not exceed the size of sun_path.
> * The addrlen argument that describes the enclosing sockaddr_un structure should have a value of at least:
>
> offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1
>
> or, more simply, addrlen can be specified as `sizeof(struct sockaddr_un)`.
Ah, so I see, line 409, it's exactly `sockaddr_un`. So all's good.
How about maybe:
```suggestion
if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
```
? And similarly 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] [thrift] ulidtko commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
ulidtko commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485688441
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
Review comment:
Null termination, whether needed or not, is covered by the `memset`. Zero out the whole structure before use — that's the best way to go. So, null termination is *obviously correct* here.
Now, I think @deiv has a point too. The `addrlen` argument to `bind` may be incorrect. @emmenlau can you check if with this patch socket names in `/proc/net/unix` contain unexpected zero-bytes? Tests would still work this way I'm sure, but it's better to double-check this.
----------------------------------------------------------------
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] [thrift] deiv commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
deiv commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485704220
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
Review comment:
Yes, as I say, the problem is the size not the null terminating character. But if you prefer to fill it with zeros, without needing it, go ahead !!
----------------------------------------------------------------
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] [thrift] emmenlau closed pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
emmenlau closed pull request #2234:
URL: https://github.com/apache/thrift/pull/2234
----------------------------------------------------------------
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] [thrift] ulidtko commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
ulidtko commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485657206
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(address)))) {
Review comment:
well I didn't check this in headers, but the manpage defines it as:
```
struct sockaddr_un {
sa_family_t sun_family; /* AF_UNIX */
char sun_path[108]; /* Pathname */
};
```
so it has fixed space for the socket path. I'm not sure the previous code was correct — but this PR seems to be OK. Also, you run tests with this patch, right?
----------------------------------------------------------------
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] [thrift] emmenlau commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485674558
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
Review comment:
Can you elaborate further if you observed problems with this? I had the same problems as https://stackoverflow.com/a/11640827/7200132 and problems went away when switching to above solution. This was tested on Ubuntu 18.04 and CentOS 7.
----------------------------------------------------------------
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] [thrift] ulidtko commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
ulidtko commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485661899
##########
File path: lib/cpp/src/thrift/transport/TSocket.cpp
##########
@@ -329,31 +329,27 @@ void TSocket::openConnection(struct addrinfo* res) {
if (!path_.empty()) {
#ifndef _WIN32
- size_t len = path_.size() + 1;
- if (len > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
+ if ((path_.size() + 1) > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() Unix Domain socket path too long", errno_copy);
throw TTransportException(TTransportException::NOT_OPEN, " Unix Domain socket path too long");
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
Review comment:
I think I get it.
This code would "truncate" the unused bytes from `address.sun_path` (which'd include the null terminator BTW).
This may have worked — but `unix(7)` clearly says to pass `sizeof(struct sockaddr_un)`, the full thing, without any truncation business.
----------------------------------------------------------------
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] [thrift] emmenlau commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485654094
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(address)))) {
Review comment:
After re-considering #2235, I think this PR is wrong. I'm not handling the abstract socket paths correctly since they indeed may require `structlen` rather than `static_cast<socklen_t>(sizeof(struct sockaddr_un))`. What do you think?
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(address)))) {
Review comment:
After re-considering #2235, I think my PR is wrong. I'm not handling the abstract socket paths correctly since they indeed may require `structlen` rather than `static_cast<socklen_t>(sizeof(struct sockaddr_un))`. What do you think?
----------------------------------------------------------------
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] [thrift] deiv commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
deiv commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485681847
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
Review comment:
As the manual page says:
> The socket's address in this namespace is given by the
> additional bytes in sun_path that are covered by the specified
> length of the address structure. (Null bytes in the name have no
> special significance.)
For abstract socket you need to pass the length of the string (without the null termination), plus sockaddr_un.sun_family size.
`static_cast<socklen_t>(sizeof(struct sockaddr_un)))` I think that will give the entire struct size.
In my case, Debian testing, for the null termination I'm getting an @. So if I want to connect to the socket from Java, I need to put extras '\0' at the final of the asbtract socket id to make it work.
----------------------------------------------------------------
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] [thrift] deiv commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
deiv commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485670897
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
Review comment:
I think, this will break the abstract unix socket type name.
----------------------------------------------------------------
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] [thrift] emmenlau commented on pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#issuecomment-690161502
This commit is now superseded by #2235
----------------------------------------------------------------
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] [thrift] emmenlau commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485656336
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(address)))) {
Review comment:
Ok, refreshing my mind with https://stackoverflow.com/a/11640827/7200132, I take back the previous comment. It does work as I proposed, and zero-ing the full path is correct and necessary. So I'll consider your suggestion!
----------------------------------------------------------------
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] [thrift] deiv commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets
Posted by GitBox <gi...@apache.org>.
deiv commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485681847
##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
}
struct sockaddr_un address;
+ // Store the zero-terminated path in address.sun_path
+ memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
- memcpy(address.sun_path, path_.c_str(), len);
-
- auto structlen = static_cast<socklen_t>(sizeof(address));
+ memcpy(address.sun_path, path_.c_str(), path_.size());
if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
- // sun_path is not null-terminated in this case and structlen determines its length
- structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
throw TTransportException(TTransportException::NOT_OPEN,
" Abstract Namespace Domain socket path not supported");
#endif
}
do {
- if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+ if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
Review comment:
As the manual page says:
> The socket's address in this namespace is given by the
> additional bytes in sun_path that are covered by the specified
> length of the address structure. (Null bytes in the name have no
> special significance.)
For abstract socket you need to pass the length of the string (without the null termination), plus sockaddr_un.sun_family size.
`static_cast<socklen_t>(sizeof(struct sockaddr_un)))` I think that will give the entire struct size
----------------------------------------------------------------
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