Fix abstract unix socket name #2235

deiv · 2020-09-09T14:13:42Z

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.

emmenlau · 2020-09-09T14:15:49Z

But len is already path_.size() + 1, does that not compensate for the null-termination?

ulidtko · 2020-09-09T14:29:58Z

This is going to conflict with #2234 — @deiv would you help with review there?

deiv · 2020-09-09T14:36:54Z

@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:

...
new TServerSocket("\0abstract-socket");
...

Searching for it in '/proc' `cat /proc/net/unix | grep abstract-', gives:

... @abstract-socket@

The last '@' is extra, due to the null char path_.c_str(), returns a null terminated string.

ulidtko · 2020-09-09T14:44:51Z

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.

deiv · 2020-09-09T14:51:18Z

@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).

ulidtko · 2020-09-09T15:01:01Z

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.

deiv · 2020-09-09T15:08:01Z

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 ?

ulidtko · 2020-09-09T15:18:22Z

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?

ulidtko · 2020-09-09T15:23:30Z

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.

deiv · 2020-09-09T15:25:33Z

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.

deiv · 2020-09-09T15:27:29Z

memcpy(address.sun_path, path_.c_str(), path_.size());

is memcpy(address.sun_path, path_.c_str(), len)

emmenlau · 2020-09-09T15:32:59Z

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.

deiv · 2020-09-09T15:33:29Z

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.

emmenlau · 2020-09-09T15:36:55Z

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:

#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

deiv · 2020-09-09T15:39:58Z

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:
#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.

ulidtko · 2020-09-09T15:44:06Z

@emmenlau all good as you said, per my understanding.

I'd update the comment too, maybe like this.

#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.

deiv · 2020-09-09T15:44:17Z

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.

ulidtko · 2020-09-09T15:47:14Z

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(...);
    }

deiv · 2020-09-09T15:49:25Z

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.

emmenlau · 2020-09-09T15:51:17Z

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.

deiv · 2020-09-09T15:53:26Z

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 the standard.

Is simply, the problem is around checking correctly the size and the terminating null; there's no need for zeroing all the struct and lost some useful cpu cycles (but if you are more confident this way ...).

ulidtko · 2020-09-09T15:57:05Z

(there's no need for zeroing all the struct)

Disagreed here. Technically, you're right*, but maintenance-wise and for future-proofing (and not sending stack garbage directly into the kernel — for abstract sockets), it's best to zero it out.

*with conditions

deiv · 2020-09-09T15:59:40Z

(there's no need for zeroing all the struct)

Disagreed here. Technically, you're right*, but maintenance-wise and for future-proofing (and not sending stack garbage directly into the kernel — for abstract sockets), it's best to zero it out.

*with conditions

The zeros as the manual states, have no special significance, so they are garbage too, so no point here.

And for maintenance point, better to have the correct handling in code, instead of try to fix it, as zeoing something.

emmenlau · 2020-09-09T16:02:11Z

(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.

deiv · 2020-09-09T16:05:43Z

(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),

emmenlau · 2020-09-09T16:08:09Z

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.

emmenlau · 2020-09-09T16:09:08Z

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? :-)

deiv · 2020-09-09T16:12:51Z

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.

ulidtko · 2020-09-09T16:13:58Z

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.

    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!

deiv · 2020-09-09T16:17:43Z

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.

    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 :)

ulidtko · 2020-09-09T16:24:27Z

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.

deiv · 2020-09-09T16:35:24Z

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.

emmenlau · 2020-09-09T17:57:07Z

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?

emmenlau · 2020-09-09T17:59:24Z

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 ifis the cleanest solution. @deiv would you accept a solution based on if? It would be awesome if you can propose something...

deiv · 2020-09-10T10:07:53Z

@ulidtko there goes

emmenlau · 2020-09-10T10:29:30Z

@ulidtko there goes

This newest version looks very good to me. Nice work.

ulidtko

Awesome, this looks pretty good!

deiv · 2020-09-10T10:46:59Z

@ulidtko @emmenlau

thanks (the twoo). Interesting discussion hehe :)

deiv · 2020-09-10T11:31:48Z

@ulidtko

do you known when we get a new release ?

emmenlau · 2020-09-10T11:58:29Z

@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.

Jens-G · 2020-09-12T17:25:24Z

do you known when we get a new release ?

The de-facto truth is: When somebody^*) manages some time to start working on one.

_{*) out of a group of certain people, not just anybody}

zeshuai007 · 2020-09-21T03:08:59Z

It build fail in CI. see this

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 ?

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 not add the termination null byte.

deiv · 2020-09-21T10:29:50Z

@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

I don't have a windows development enviroment, so I can't take a look.

emmenlau · 2020-09-21T11:06:34Z

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

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?

deiv · 2020-09-21T11:26:26Z

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

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.

emmenlau · 2020-09-21T12:22:22Z

Good, I agree! Then @zeshuai007 please review!

emmenlau mentioned this pull request Sep 9, 2020

Improve setting the domain socket path into the sockaddr_un struct in TSockets #2234

Closed

5 tasks

deiv force-pushed the fix-abstract-unix-socket-name branch from 90d56db to 4fa63e7 Compare September 10, 2020 10:06

deiv force-pushed the fix-abstract-unix-socket-name branch from 4fa63e7 to 52e88cb Compare September 10, 2020 10:12

ulidtko approved these changes Sep 10, 2020

View reviewed changes

Fix abstract unix socket name

4750f36

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 not add the termination null byte.

deiv force-pushed the fix-abstract-unix-socket-name branch from 52e88cb to 4750f36 Compare September 21, 2020 09:37

zeshuai007 merged commit 03f4729 into apache:master Sep 22, 2020

Fix abstract unix socket name #2235

Fix abstract unix socket name #2235

Conversation

deiv commented Sep 9, 2020

emmenlau commented Sep 9, 2020

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020 • edited

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020

ulidtko commented Sep 9, 2020

ulidtko commented Sep 9, 2020 • edited

deiv commented Sep 9, 2020

deiv commented Sep 9, 2020 • edited

emmenlau commented Sep 9, 2020 • edited

deiv commented Sep 9, 2020

emmenlau commented Sep 9, 2020

deiv commented Sep 9, 2020 • edited

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020 • edited

emmenlau commented Sep 9, 2020

deiv commented Sep 9, 2020 • edited

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020

emmenlau commented Sep 9, 2020

deiv commented Sep 9, 2020

emmenlau commented Sep 9, 2020

emmenlau commented Sep 9, 2020

deiv commented Sep 9, 2020

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020

ulidtko commented Sep 9, 2020

deiv commented Sep 9, 2020

emmenlau commented Sep 9, 2020

emmenlau commented Sep 9, 2020

deiv commented Sep 10, 2020

emmenlau commented Sep 10, 2020

ulidtko left a comment

Choose a reason for hiding this comment

deiv commented Sep 10, 2020

deiv commented Sep 10, 2020

emmenlau commented Sep 10, 2020

Jens-G commented Sep 12, 2020

zeshuai007 commented Sep 21, 2020

deiv commented Sep 21, 2020

emmenlau commented Sep 21, 2020

deiv commented Sep 21, 2020

emmenlau commented Sep 21, 2020

deiv commented Sep 9, 2020 •

edited

ulidtko commented Sep 9, 2020 •

edited

deiv commented Sep 9, 2020 •

edited

emmenlau commented Sep 9, 2020 •

edited

deiv commented Sep 9, 2020 •

edited

deiv commented Sep 9, 2020 •

edited

deiv commented Sep 9, 2020 •

edited