search

hugelgupf / p9

Idiomatic Go 9P2000.L client and server, extracted from gVisor for general use
Apache License 2.0
87 stars 19 forks source link

Regression in `ReadAt` causes 0'd payloads #65

Closed djdv closed 1 year ago

djdv commented 1 year ago

78606c499424d4fdbaf2f804932c28beea103b5a causes a regression in ReadAt where it will return zeros instead of the actual data that was read.

I have a reproducible here: pipe.go which works on tag v0.2.0 but not on 5cb68b4d03bb7e29a9e8b47fd68537204ace21f7. As well as output before and after applying a patch that seems to fix it. (Edit: previous patch was not go fmt'd)

If the patch seems correct, I can submit a PR, but I'm unsure if this is the actually the right approach. Would appreciate feedback on it.

While debugging, I found this: A response Message is instantiated here as rread and is passed to a call which will eventually call recv: https://github.com/hugelgupf/p9/blob/5cb68b4d03bb7e29a9e8b47fd68537204ace21f7/p9/client_file.go#L308-L309

The type assertion within recv fails here (and thus the payload is ignored, even if it was read successfully): https://github.com/hugelgupf/p9/blob/5cb68b4d03bb7e29a9e8b47fd68537204ace21f7/p9/transport.go#L197

Implementing the payloader methods on rread resolves this, but so does using rreadPayloader inside of the (c *clientFile) readAt call. (However, we can't populate .fullBuffer nor .cs in that scope so that's probably not the right solution.)

This also doesn't seem to happen when using tcp sockets, or at least I didn't witness it. So I'm not really sure what's going on. The only thing that comes to mind is that they're handled differently in vecnet's ReadFrom, but this doesn't seem like it would influence which message type is used. Not sure why this is the case. https://github.com/hugelgupf/p9/blob/5cb68b4d03bb7e29a9e8b47fd68537204ace21f7/vecnet/vecnet.go#L35-L36

Extra: I discovered this problem after updating one of my utilities that creates a subprocess and communicates with it over interfaces that wrap stdio (if they're not wrapped, vecnet's ReadFrom tries to use them as syscall.Conn because *os.File implements that, which eventually fails with "socket operation on non-socket"/ENOTSOCK).

hugelgupf commented 1 year ago

If I interpret this correctly, you're saying this is a regression on the clientFile right? I've been pretty neglectful in testing that adequately. It's pretty much untested. I'll try to take a closer look this week.

djdv commented 1 year ago

this is a regression on the clientFile right?

I believe so, specifically starting around clientFile.readAt. IIRC the client performs the operation as expected, but the payload gets ignored in recv because the message type doesn't implement payloader.

This problem doesn't seem to happen with TCP net.Conns, only some other kinds of io.ReadWriteClosers. Specifically os.Pipe, which I use to speak 9P with a child process via stdio, and net.Pipe for the repro above. This worked before, broke in 5cb68b4d03bb7e29a9e8b47fd68537204ace21f7, and works again when this diff is applied. I understand why that fixes it but I don't know why it's not broken for TCP, or if there's a more proper way to fix it.

I'll try to take a closer look this week.

Thanks :^] If you need any help from me, just let me know.

hugelgupf commented 1 year ago

I am actually puzzled, now looking at it, how the client would work at all (pipe or not).

hugelgupf commented 1 year ago

I don't have much I added in the way of tests, but I sent #82.