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:13:50 UTC
[GitHub] [thrift] deiv opened a new pull request #2235: Fix abstract unix socket name
deiv opened a new pull request #2235:
URL: https://github.com/apache/thrift/pull/2235
For the abstract unix socket address type, the string in the
'sun_path' field of the 'sockaddr_un struct', is a not null-terminated
string (see unix(7)).
Fix the lentgh calculation of the 'sun_path' field to don't add
the termination null byte.
----------------------------------------------------------------
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] Jens-G commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-691520153
> do you known when we get a new release ?
The de-facto truth is: When somebody<sup>*)</sup> manages some time to start working on one.
<sub>*) out of a group of certain people, not just anybody</sub>
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-696055493
> > I added a TODO, to take into account, that windows has support to unix sockets, but the code is not prepared for that. At least it needs to include the windows headers for unix sockets. See [af_unix-comes-to-windows](https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/)
>
> I have PR #2127 that adds support for Unix sockets under Windows. Generally its positive to have separate PRs for separate functionality, but in this case its not necessarily better that you need to guard _against_ supporting Unix sockets only for me to later add the support in a subsequent PR. So please feel free to cannibalize #2127 and take everything you need to support the Unix sockets on MSVC?
Hi @emmenlau,
I don't have a windows development enviroment, and because, to add support for Unix sockets, is needed a new header (and the checks for it), better to have it in another separate PR.
And is needed to remove other windows conditional compilation in sockets sources files, too. Eg:
```
#ifndef _WIN32
socklen_t structlen = fillUnixSocketAddr(address, path_);
```
This one only fix abstract namespace, so better if you can do another PR removing the conditional compilation for windows. Seems more logical to have the new windows unix socket support separated.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689614277
@ulidtko
The other way to do it, is calculating the len of the string based on the type of socket:
pathname socket: null terminated
abstract socket: not null terminated
This lead to an if in some point of the code. I prefer to do an aritmethic operation instead of adding a conditional jump (for a cpu point of view, the arithmethic way will not lead to control hazards).
----------------------------------------------------------------
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] zeshuai007 commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
zeshuai007 commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-695881732
It build fail in CI. see [this](https://ci.appveyor.com/project/ApacheSoftwareFoundation/thrift/builds/35128105/job/5alf601ondssa2vy)
AppVeryor log show like this.
```
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(52): error C2065: 'is_abstract_namespace': undeclared identifier [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(65): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(65): error C2227: left of '->sun_path' must point to class/struct/union/generic type [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(71): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(71): error C2228: left of '.sun_family' must have class/struct/union [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(72): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(72): error C2228: left of '.sun_path' must have class/struct/union [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(74): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(74): error C2227: left of '->sun_family' must point to class/struct/union/generic type [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
```
Could you build success in your local environment ?
----------------------------------------------------------------
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 edited a comment on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko edited a comment on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689635143
> we are using `c_str()' that returns an null terminated string
`c_str()` does return a null-terminated C string... but the `memcpy` is not copying it into the `sun_path`, due to `memcpy(address.sun_path, path_.c_str(), path_.size());` — `.size()` does not include the last null.
--------------------
*edit: sorry I confused this from the other PR. code in master does
```
size_t len = path_.size() + 1;
// ...
memcpy(address.sun_path, path_.c_str(), len);
```
so it's fine.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689620808
OK, so we have:
* pathname socket — `'\0'` is last
* abstract socket — `'\0'` is first
The `.sun_path` length is `path_.size() + 1` in both cases. This needs no `if`-s...
I mean: the `structlen -= …` line is bad, one needs to think really hard to understand it completely. While the PR needs to change it anyway — why not improve it? The `+ 1` you're adding does fix *the issue*, but makes *the code* worse.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689635143
> we are using `c_str()' that returns an null terminated string
`c_str()` does return a null-terminated C string... but the `memcpy` is not copying it into the `sun_path`, due to `memcpy(address.sun_path, path_.c_str(), path_.size());` — `.size()` does not include the last null.
----------------------------------------------------------------
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 edited a comment on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv edited a comment on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689605217
I think is the other way around:
The `path_.size() + 1` adds 1 to compesate for the null termination needed in the "pathname-based" unix socket type.
If for example, you create an abstract unix socket server in thtift:
```c++
...
new TServerSocket("\0abstract-socket");
...
```
Searching for it in '/proc' `cat /proc/net/unix | grep abstract-', gives:
```bash
... @abstract-socket@
```
The last '@' is extra, due to the null char `path_.c_str()`, returns a null terminated string.
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689643457
Let me see if I get everything right:
- From my side, it would be fine to pass the shorter struct length in case of abstract sockets. For me it worked to use the _full_ length, but I think it should be fine to use the shorter length.
- The `memcpy` and `memset` are good for everybody? So we copy only the required bytes but ensure the full struct is zero'ed before use.
So the "only" challenge is to agree on a good style to compute `structlen`, correct? I personally would prefer something like:
```cpp
#ifdef __linux__
// sun_path is not null-terminated in this case, and structlen must be shortened to the path size
structlen = static_cast<socklen_t>(sizeof(address)) - sizeof(address.sun_path) + path_.size();
#else
```
----------------------------------------------------------------
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] Jens-G commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-691520153
> do you known when we get a new release ?
The de-facto truth is: When somebody<sup>*)</sup> manages some time to start working on one.
<sub>*) out of a group of certain people, not just anybody</sub>
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689631960
> we need only path_.size()
so you're saying, this would be correct call?
```
sockaddr_un addr;
addr.sun_family = AF_UNIX;
addr.sun_path[0] = '\0';
addr.sun_path[1] = 'A';
addr.sun_path[2] = 'B';
addr.sun_path[3] = 'C';
bind(sockfd, &addr, 3);
// ^ here, 3? not 4?
```
> Maybe a comment could do the job ?
As you can see, there's already one. Is it helpful?
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689663413
> For me I see zeroing them as an unneed operation, that mask the correct solution that is, better handling of null terminated string. (but c and c++ are always hard),
Ok, lets settle for "would you be ok to leave the zeroing in place"? It does not hurt much, if nothing else? :-)
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-696030777
@zeshuai007
Fixed, seems that thrift is not ready for compiling unix sockets under windows. I just added some conditional macros.
I fixed too, a variable that does not has the correct name (`is_abstract_namespace`)
I added a TODO, to take into account, that windows has support to unix sockets, but the code is not prepared for that. At least it needs to include the windows headers for unix sockets. See [af_unix-comes-to-windows](https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/)
I don't have a windows development enviroment, so I can't take a look.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689636406
> > we need only path_.size()
>
> so you're saying, this would be correct call?
>
> ```
> sockaddr_un addr;
> addr.sun_family = AF_UNIX;
> addr.sun_path[0] = '\0';
> addr.sun_path[1] = 'A';
> addr.sun_path[2] = 'B';
> addr.sun_path[3] = 'C';
>
> bind(sockfd, &addr, 3);
> // ^ here, 3? not 4?
> ```
Not, is 4, but when you will do `"\0ABC".size() + 1`, it will gives 5.
> > Maybe a comment could do the job ?
>
> As you can see, there's already one. Is it helpful?
The comment is:
> // sun_path is not null-terminated in this case and structlen determines its length
Yes, is helpful, it's give the correct way to do.
But the code was not the correct, because it uses the variable `len` that is the length of the string plus the terminating caracter \0.
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689592088
But `len` is already `path_.size() + 1`, does that not compensate for the null-termination?
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689723230
> Make a unique function call for the 3 files, could be good too ... :)
This is actually a subset of a larger section of TServerSocket.cpp and TSocket.cpp that mostly overlaps. For maintenance this would much better be unified. I don't have too much spare time, anyone wants to give it a go?
----------------------------------------------------------------
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] zeshuai007 merged pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
zeshuai007 merged pull request #2235:
URL: https://github.com/apache/thrift/pull/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] ulidtko commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689600917
This is going to conflict with #2234 — @deiv would you help with review there?
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689625283
> OK, so we have:
>
> * pathname socket — `'\0'` is last
> * abstract socket — `'\0'` is first
>
> The `.sun_path` length is `path_.size() + 1` in both cases. This needs no `if`-s...
>
* pathname socket — `'\0'` is last -> we need `path_.size() + 1`
* abstract socket — `'\0'` is first -> we don't need `path_.size() + 1` (we need only `path_.size() `
Take into account that, for the `memcpy(address.sun_path, path_.c_str(), len)` thing, we are using `c_str()' that returns an null terminated string
> I mean: the `structlen -= …` line is bad, one needs to think really hard to understand it completely. While the PR needs to change it anyway — why not improve it? The `+ 1` you're adding does fix _the issue_, but makes _the code_ worse.
Maybe a comment could do the job ?
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-690130924
@ulidtko there goes
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689605217
I think is the other way around:
The `path_.size() + 1` adds 1 to compesate for the null termination needed in the "pathname-based" unix socket type.
If for example, you create an abstract unix socket server in thtift:
```c++
...
new TServerSocket("\0abstract-socket");
...
```
Searching for it in '/proc' `cat /proc/net/unix | grep abstract-', gives:
```bash
... @abstract-socket@
```
The last '@' is extra, due to the null char `path_.c_str(), len)`, return a null terminated string.
----------------------------------------------------------------
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 edited a comment on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv edited a comment on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689651089
> > if the string is the same size in bytes as the 'sockaddr_un.sun_path', you will lack the null terminated string, and posibly getting a memory error.
>
> This won't happen, due to
>
> ```
> size_t len = path_.size() + 1;
> if (len > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
> ...
> throw TTransportException(...);
> }
> ```
If the path_is for an abstract unix socket, the `size_t len = path_.size() + 1' is not ok, is legal to fill the entire address.sun_path field.
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689724316
> I think, the `if` for abstract/path is unavoidable — and arguments to both `memcpy` and `bind` will differ depending on that. Trying to unify the branches of that `if` leads straight into confusion.
The longer I think about it, yes maybe the `if`is the cleanest solution. @deiv would you accept a solution based on `if`? It would be awesome if you can propose something...
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-690220580
> @ulidtko
>
> do you known when we get a new release ?
There is no real schedule, and no release plan. So I would not hold my breath for it. If you are very curious or urgently need a release, I would suggest to bring the topic to the mailing list.
----------------------------------------------------------------
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 edited a comment on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv edited a comment on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689637601
> memcpy(address.sun_path, path_.c_str(), path_.size());
is `memcpy(address.sun_path, path_.c_str(), len)`
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689662859
> > But then copying the zero-termination would write beyond the end of `sockaddr_un.sun_path`. This is why I prefer _not_ to copy the zero-termination.
>
> In the case of pathname socket yes, for abstract unix socket is legal, because you don't need to copy the null terminating. is the standard.
But this is more error-prone and complex than to just zero the full struct, and then copy _only_ `path_.size()` char's, isn't it? The latter proposed approach works for all cases (pathname sockets and abstract sockets), and it supports your suggested use case where the abstract socket uses the full `sun_path` without trailing zero-termination.
----------------------------------------------------------------
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] zeshuai007 merged pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
zeshuai007 merged pull request #2235:
URL: https://github.com/apache/thrift/pull/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] deiv edited a comment on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv edited a comment on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689605217
@emmenlau: I think is the other way around:
The `path_.size() + 1` adds 1 to compesate for the null termination needed in the "pathname-based" unix socket type.
If for example, you create an abstract unix socket server in thtift:
```c++
...
new TServerSocket("\0abstract-socket");
...
```
Searching for it in '/proc' `cat /proc/net/unix | grep abstract-', gives:
```bash
... @abstract-socket@
```
The last '@' is extra, due to the null char `path_.c_str()`, returns a null terminated string.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-690150200
@ulidtko @emmenlau
thanks (the twoo). Interesting discussion hehe :)
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689672464
> Make a unique function call for the 3 files, could be good too ... :)
Well, not such a bad idea, haha. Will you do it?
> And, the memset, for me, is not making nothing :)
I understand; let's just agree to disagree on this, can we?
-----------------------
Also, as a reminder: we have 2 PR's fixing the same thing in different ways. #2234 and #2235.
It's best to focus on one and close the other.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689668535
> I think, the `if` for abstract/path is unavoidable — and arguments to both `memcpy` and `bind` will differ depending on that. Trying to unify the branches of that `if` leads straight into confusion.
>
> ```c++
> struct sockaddr_un address;
> memset(&address, '\0', sizeof(address));
> address.sun_family = AF_UNIX;
>
> if (!path_[0]) { // "abstract" namespace socket (Linux-specific)
>
> // space check
> if (path_.size() > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
> throw TTransportException(...);
> }
>
> // no null termination here
> memcpy(address.sun_path, path_.c_str(), path_.size());
> addrlen = path._size();
>
> } else { // "pathname" socket
>
> // space check, with additional byte for the terminator
> if (path_.size() + 1 > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
> throw TTransportException(...);
> }
>
> // including the null terminator
> strncpy(address.sun_path, path_.c_str(), path_.size() + 1);
> addrlen = path._size() + 1;
> }
>
> ::bind(/*…*/, /*…*/, addrlen);
> ```
>
> @deiv
>
> > ... So better to do a refactoring.
>
> Yes exactly — that's why I'm saying I'm not totally happy with your patch!
Make a unique function call for the 3 files, could be good too ... :)
And, the memset, for me, is not making nothing :)
----------------------------------------------------------------
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] Jens-G commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-691520153
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689647891
> > > memcpy(address.sun_path, path_.c_str(), path_.size());
> >
> >
> > is `memcpy(address.sun_path, path_.c_str(), len)`
>
> I think its better to copy only `path_size()` instead of `len` because it avoids copying a char that is not in the original string.
>
> Zero-termination is (better?) handled by the `memset`.
Not, if the string is the same size in bytes as the 'sockaddr_un.sun_path', you will lack the null terminated string, and posibly getting a memory error.
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-696046638
> I added a TODO, to take into account, that windows has support to unix sockets, but the code is not prepared for that. At least it needs to include the windows headers for unix sockets. See [af_unix-comes-to-windows](https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/)
I have PR #2127 that adds support for Unix sockets under Windows. Generally its positive to have separate PRs for separate functionality, but in this case its not necessarily better that you need to guard *against* supporting Unix sockets only for me to later add the support in a subsequent PR. So please feel free to cannibalize #2127 and take everything you need to support the Unix sockets on MSVC?
----------------------------------------------------------------
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] zeshuai007 commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
zeshuai007 commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-695881732
It build fail in CI. see [this](https://ci.appveyor.com/project/ApacheSoftwareFoundation/thrift/builds/35128105/job/5alf601ondssa2vy)
AppVeryor log show like this.
```
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(52): error C2065: 'is_abstract_namespace': undeclared identifier [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(65): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(65): error C2227: left of '->sun_path' must point to class/struct/union/generic type [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(71): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(71): error C2228: left of '.sun_family' must have class/struct/union [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(72): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(72): error C2228: left of '.sun_path' must have class/struct/union [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(74): error C2027: use of undefined type 'apache::thrift::transport::sockaddr_un' [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(46): note: see declaration of 'apache::thrift::transport::sockaddr_un'
C:\projects\thrift\lib\cpp\src\thrift\transport\SocketCommon.cpp(74): error C2227: left of '->sun_family' must point to class/struct/union/generic type [C:\projects\build\MSVC2015\x86\lib\cpp\thrift.vcxproj]
```
Could you build success in your local environment ?
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689666330
I think, the `if` for abstract/path is unavoidable — and arguments to both `memcpy` and `bind` will differ depending on that. Trying to unify the branches of that `if` leads straight into confusion.
```c++
struct sockaddr_un address;
memset(&address, '\0', sizeof(address));
address.sun_family = AF_UNIX;
if (!path_[0]) { // "abstract" namespace socket (Linux-specific)
// space check
if (path_.size() > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
throw TTransportException(...);
}
// no null termination here
memcpy(address.sun_path, path_.c_str(), path_.size());
addrlen = path._size();
} else { // "pathname" socket
// space check, with additional byte for the terminator
if (path_.size() + 1 > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
throw TTransportException(...);
}
// including the null terminator
strncpy(address.sun_path, path_.c_str(), path_.size() + 1);
addrlen = path._size() + 1;
}
::bind(/*…*/, /*…*/, addrlen);
```
-----------------------------
@deiv
> ... So better to do a refactoring.
Yes exactly — that's why I'm saying I'm not totally happy with your patch!
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-690193083
@ulidtko
do you known when we get a new release ?
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-696030777
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689651089
> > if the string is the same size in bytes as the 'sockaddr_un.sun_path', you will lack the null terminated string, and posibly getting a memory error.
>
> This won't happen, due to
>
> ```
> size_t len = path_.size() + 1;
> if (len > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
> ...
> throw TTransportException(...);
> }
> ```
If the path_ if for a abstract unix socket, the `size_t len = path_.size() + 1' is not ok, is legal to fill the entire address.sun_path field.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689637601
> memcpy(address.sun_path, path_.c_str(), path_.size());
is `memcpy(address.sun_path, path_.c_str(), path_.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
[GitHub] [thrift] emmenlau commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-696079005
Good, I agree! Then @zeshuai007 please review!
----------------------------------------------------------------
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] zeshuai007 merged pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
zeshuai007 merged pull request #2235:
URL: https://github.com/apache/thrift/pull/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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-690141003
> @ulidtko there goes
This newest version looks very good to me. Nice 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] emmenlau commented on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-696046638
----------------------------------------------------------------
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 edited a comment on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv edited a comment on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689645345
> Let me see if I get everything right:
>
> * From my side, it would be fine to pass the shorter struct length in case of abstract sockets. For me it worked to use the _full_ length, but I think it should be fine to use the shorter length.
> * The `memcpy` and `memset` are good for everybody? So we copy only the required bytes but ensure the full struct is zero'ed before use.
>
The `memcpy(address.sun_path, path_.c_str(), path_.size());` is not correct, think that you need always a null terminated string. path.size() return the bytes count in the string, not the null termination.
> So the "only" challenge is to agree on a good style to compute `structlen`, correct? I personally would prefer something like:
>
> ```c++
> #ifdef __linux__
> // sun_path is not null-terminated in this case, and structlen must be shortened to the path size
> structlen = static_cast<socklen_t>(sizeof(address)) - sizeof(address.sun_path) + path_.size();
> #else
> ```
The code need to be fixed in 3 files (not 2). and the code is the same in the 3 files, So better to do a refactoring.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689678533
> > Make a unique function call for the 3 files, could be good too ... :)
>
> Well, not such a bad idea, haha. Will you do it?
Can take a look tomorrow. If nobody want to do after.
> > And, the memset, for me, is not making nothing :)
>
> I understand; let's just agree to disagree on this, can we?
uhm... xD
> Also, as a reminder: we have 2 PR's fixing the same thing in different ways. #2234 and #2235.
>
> It's best to focus on one and close the other.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689665674
> > > But then copying the zero-termination would write beyond the end of `sockaddr_un.sun_path`. This is why I prefer _not_ to copy the zero-termination.
> >
> >
> > In the case of pathname socket yes, for abstract unix socket is legal, because you don't need to copy the null terminating. is the standard.
>
> But this is more error-prone and complex than to just zero the full struct, and then copy _only_ `path_.size()` char's, isn't it? The latter proposed approach works for all cases (pathname sockets and abstract sockets), and it supports your suggested use case where the abstract socket uses the full `sun_path` without trailing zero-termination.
Yes, more easy, this way, you forget about the terminating string. But the question is: If I go to a fountain because I need one liter of water, and I fill five just in case (is the correct way o not?). Points of view.
But if the code need's to handling the null terminating and the size, and the two are correct. Removing the memset, will not make any difference.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689653546
> > > > if the string is the same size in bytes as the 'sockaddr_un.sun_path', you will lack the null terminated string, and posibly getting a memory error.
> > >
> > >
> > > This won't happen, due to
> > > ```
> > > size_t len = path_.size() + 1;
> > > if (len > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
> > > ...
> > > throw TTransportException(...);
> > > }
> > > ```
> >
> >
> > If the path_is for an abstract unix socket, the `size_t len = path_.size() + 1' is not ok, is legal to fill the entire address.sun_path field.
>
> But then copying the zero-termination would write beyond the end of `sockaddr_un.sun_path`. This is why I prefer _not_ to copy the zero-termination.
In the case of pathname socket yes, for abstract unix socket is legal, because you don't need to copy the null terminating.
Is simply, the problem is around checking correctly the size and the terminating null (there's no need for zeroing all the struct).
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689610154
That actually makes sense. Hmmm...
But regardless, I'd not be happy with the patch itself. ` structlen -= sizeof(address.sun_path) - len + 1;` — this approaches the comprehension limit for me (read: *this guy is stupid, doesn't grok the elite `-=-` operator*).
Could this be solved in a more clear way? Y'know, such that it'd be obviously-correct.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689649749
> if the string is the same size in bytes as the 'sockaddr_un.sun_path', you will lack the null terminated string, and posibly getting a memory error.
This won't happen, due to
```
size_t len = path_.size() + 1;
if (len > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
...
throw TTransportException(...);
}
```
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689647791
@emmenlau all good as you said, per my understanding.
I'd update the comment too, maybe like this.
```C++
#ifdef __linux__
// sun_path is not null-terminated for abstract sockets, truncate structlen to the path size
structlen = static_cast<socklen_t>(sizeof(address) - sizeof(address.sun_path) + path_.size());
#else
```
Also, maybe move the `static_cast<socklen_t>` to the expr top-level, to not mess up integer overflows in the arithmetic.
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689652182
> > > if the string is the same size in bytes as the 'sockaddr_un.sun_path', you will lack the null terminated string, and posibly getting a memory error.
> >
> >
> > This won't happen, due to
> > ```
> > size_t len = path_.size() + 1;
> > if (len > sizeof(((sockaddr_un*)nullptr)->sun_path)) {
> > ...
> > throw TTransportException(...);
> > }
> > ```
>
> If the path_is for an abstract unix socket, the `size_t len = path_.size() + 1' is not ok, is legal to fill the entire address.sun_path field.
But then copying the zero-termination would write beyond the end of `sockaddr_un.sun_path`. This is why I prefer _not_ to copy the zero-termination.
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689659076
> (there's no need for zeroing all the struct)
Actually, it sadly _is_ required. See the stackoverflow link from above (and also my own experience). The kernel _may_ use all characters in `sun_path` for the socket name, which can add funny characters if there is random data in the variable. This actually happened to me.
----------------------------------------------------------------
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 #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689641015
> > memcpy(address.sun_path, path_.c_str(), path_.size());
>
> is `memcpy(address.sun_path, path_.c_str(), len)`
I think its better to copy only `path_size()` instead of `len` because it avoids copying a char that is not in the original string.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689645345
> Let me see if I get everything right:
>
> * From my side, it would be fine to pass the shorter struct length in case of abstract sockets. For me it worked to use the _full_ length, but I think it should be fine to use the shorter length.
> * The `memcpy` and `memset` are good for everybody? So we copy only the required bytes but ensure the full struct is zero'ed before use.
>
The `memcpy(address.sun_path, path_.c_str(), path_.size());` is not correct, think that you need always a null terminated string. path.size() return the bytes count in the string, not the null termination.
> So the "only" challenge is to agree on a good style to compute `structlen`, correct? I personally would prefer something like:
>
> ```c++
> #ifdef __linux__
> // sun_path is not null-terminated in this case, and structlen must be shortened to the path size
> structlen = static_cast<socklen_t>(sizeof(address)) - sizeof(address.sun_path) + path_.size();
> #else
> ```
----------------------------------------------------------------
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 edited a comment on pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
emmenlau edited a comment on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689641015
> > memcpy(address.sun_path, path_.c_str(), path_.size());
>
> is `memcpy(address.sun_path, path_.c_str(), len)`
I think its better to copy only `path_size()` instead of `len` because it avoids copying a char that is not in the original string.
Zero-termination is (better?) handled by the `memset`.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689641310
> > we are using `c_str()' that returns an null terminated string
>
> `c_str()` does return a null-terminated C string... but the `memcpy` is not copying it into the `sun_path`, due to `memcpy(address.sun_path, path_.c_str(), path_.size());` — `.size()` does not include the last null.
>
> *edit: sorry I confused this from the other PR. code in master does
>
> ```
> size_t len = path_.size() + 1;
> // ...
> memcpy(address.sun_path, path_.c_str(), len);
> ```
>
> so it's fine.
It's fine for the pathname socket (len includes the null). For abstract socket id not the correct, we don't need to count the null.But if you prefer to copy an additional char, go ahead. The problem is the size passed in `bind` call.
----------------------------------------------------------------
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 pull request #2235: Fix abstract unix socket name
Posted by GitBox <gi...@apache.org>.
deiv commented on pull request #2235:
URL: https://github.com/apache/thrift/pull/2235#issuecomment-689661379
> > (there's no need for zeroing all the struct)
>
> Actually, it sadly _is_ required. See the stackoverflow link from above (and also my own experience). The kernel _may_ use all characters in `sun_path` for the socket name, which can add funny characters if there is random data in the variable. This actually happened to me.
With the correct code handling , (null terminating case ok for pahtname socket, and correct lenght for abstract unix socket) the special caracters will disapear...
For me I see zeroing them as an unneed operation, that mask the correct solution that is, better handling of null terminated string. (but c and c++ are always hard),
----------------------------------------------------------------
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