OOB and disOOB implementation #2
Conversation
transport/oob/oob.go
Outdated
| oobData := []byte{w.oobByte} | ||
| var flags int | ||
| if w.disOOB { | ||
| flags = syscall.MSG_OOB | syscall.MSG_DONTROUTE // Additional flag for disOOB mode |
There was a problem hiding this comment.
I don't see MSG_OOB in the windows documentation of syscall. I believe you need https://pkg.go.dev/golang.org/x/sys/windows.
It's important for us to support Windows and have cross-platform support, so providers can specify a single strategy for all platforms.
transport/oob/oob.go
Outdated
| return nil, err | ||
| } | ||
| // Wrap connection with OOB and/or disOOB writer based on configuration | ||
| return transport.WrapConn(innerConn, innerConn, NewOOBWriter(innerConn, d.oobPosition, d.oobByte, d.disOOB)), nil |
There was a problem hiding this comment.
You should test if innerConn is a *net.TCPConn here, before wrapping.
transport/oob/oob.go
Outdated
| // NewOOBWriter creates an [io.Writer] that sends an OOB byte at the specified "oobPosition". | ||
| // If disOOB is enabled, it will apply the --disOOB strategy. | ||
| // "oobByte" specifies the value of the byte to send out-of-band. | ||
| func NewOOBWriter(writer io.Writer, oobPosition int64, oobByte byte, disOOB bool) io.Writer { |
There was a problem hiding this comment.
This can't be any writer, it must be a *TCPConn. Let's make that explicit.
transport/oob/oob.go
Outdated
There was a problem hiding this comment.
I would keep this in the x folder for now. We need comprehensive tests and an expectation of stability to be in the main SDK module. We will "graduate" experimental packages from x as they become stable and tested.
That will also remove the need for the replace directive in your go.mod file.
x/oob/ttl.go
Outdated
There was a problem hiding this comment.
Please use the new https://pkg.go.dev/github.com/Jigsaw-Code/outline-sdk/x/sockopt instead
x/oob/writer.go
Outdated
| } | ||
| } | ||
|
|
||
| err = w.send(firstPart, 0x01) |
There was a problem hiding this comment.
The flag shouldn't be hardcoded for readability and portability.
use unix.MSG_OOB and windows.MSG_OOB instead from the x/sys package.
Probably helpful to define a portable MSG_OOB flags somewhere in your code.
x/oob/writer.go
Outdated
| return &oobWriter{conn: conn, sd: sd, oobPosition: oobPosition, oobByte: oobByte, disOOB: disOOB, delay: delay} | ||
| } | ||
|
|
||
| func (w *oobWriterReaderFrom) ReadFrom(source io.Reader) (int64, error) { |
There was a problem hiding this comment.
Remove ReadFrom, since it doesn't support it.
| oobByte byte | ||
| oobPosition int64 | ||
| disOOB bool | ||
| delay time.Duration |
There was a problem hiding this comment.
Is the delay really needed? I found the TCP writes to be quite consistent and never had issues without the delay.
For simplicity, I would only add it if proven needed in Go.
There was a problem hiding this comment.
Yes. Delay is essential.
static ssize_t send_oob(int sfd, char *buffer,
ssize_t n, long pos, const char *c)
{
char rchar = buffer[pos];
buffer[pos] = c[1] ? c[0] : 'a';
ssize_t len = send(sfd, buffer, pos + 1, MSG_OOB);
buffer[pos] = rchar;
if (len < 0) {
uniperror("send");
return -1;
}
wait_send_if_support(sfd);
len--;
if (len != pos) {
return len;
}
return len;
}
This is a little bit complex implementation in byeDPI that works only on linux. So now it is only delay.
x/oob/writer.go
Outdated
| // "oobByte" specifies the value of the byte to send out-of-band. | ||
| func NewWriter( | ||
| conn *net.TCPConn, | ||
| sd SocketDescriptor, |
There was a problem hiding this comment.
No need to pass a socket descriptor. Use TCPConn.SyscallConn().Write() instead.
You shouldn't be holding socket file descriptors, because they may not be valid anymore. Go has special logic to keep them alive: https://cs.opensource.google/go/go/+/master:src/net/rawconn.go;l=57;drc=9088883cf4a5181cf796c968bbce5a5bc3edc7ab
|
|
||
| var oldTTL int | ||
| if w.disOOB { | ||
| w.setTTL.Do(func() { |
There was a problem hiding this comment.
I believe this can be accomplished by overlaying disorder on top of OOB.
Something like: oob:10:X | disorder:10
But we can figure that out later.
There was a problem hiding this comment.
In https://github.com/hufrea/byedpi/blob/a6ee6ddb924fbed33e3c4569d2ba45dcdebfc61a/desync.c#L526 this is different strategies. I will test if it possible to combine with same effect
| var err error | ||
|
|
||
| if w.oobPosition > 0 && w.oobPosition < int64(len(data)) { | ||
| firstPart := data[:w.oobPosition+1] |
There was a problem hiding this comment.
This will trigger index out of bounds if len(data) == oobPosition+1.
You may consider using https://linux.die.net/man/2/sendmsg, which supports the io vector, so you can pass two buffers for an atomic write.
No description provided.