karlseguin / websocket.zig

A websocket implementation for zig
MIT License
302 stars 28 forks source link

Potential UAF of async frame #1

Closed kprotty closed 1 year ago

kprotty commented 2 years ago

In the listen function, It starts the async frame for client.handle in a loop and binds it to a temporary variable _. Once client.handle() suspends (e.g. from partial socket read), it will return from the async call, reiterate in the while loop, and invalidate the memory the async frame is being used with.

To handle this, would recommend dynamically allocating the frames for each client.handle and deallocate the frame one handle finished. std.event.Loop.runDetached would be a good example.

karlseguin commented 2 years ago

Thanks.

I initially wrote an unfinished version. At the time I was looking at other examples, and I saw that they'd add themselves to a queue for cleanup. I even wrote a gist about it based on Andrew's version that never cleaned the client up (https://gist.github.com/karlseguin/53bb8ebf945b20aa0b7472d9d30de801).

When I picked up the project again this weekend to finish it, I thought that was pretty awful so I just abandoned it all together. Of course, now we've gone from awful to just wrong.

Enough about my life story though...

Am I able to use std.event.Loop.runDetached directly to solve this? Something like:

const stream = client.NetStream{ .stream = conn.stream };
try Loop.instance.?.runDetached(allocator, client.handle, .{ H, client.NetStream, context, stream, allocator });

Fro the runDetached function comment, it seems so. But I'd be lying if I said I really understand what was going on :(

canselcik commented 1 year ago

I wanted to give Zig another try and noticed this is the only WebSocket implementation in the language. On the current version of Zig, Loop.instance.?.runDetached seems to fail to compile. I made it build with a naive null check but seems to accept clients but not respond to them.

kprotty commented 1 year ago

@canselcik Zig temporarily removed async when it switched to stage2 post 0.10. There are plans to implement it again, but it's not yet a priority (currently, package manager & build system are). If you're looking for a websocket impl, there's also https://github.com/truemedian/wz. If that doesn't work you'll have to write it yourself (websocket protocol is surprisingly simple) or wait for someone else to do so.

karlseguin commented 1 year ago

@canselcik I created a v0.10.1-compat which is compatible with 0.10.1.

Master is now compatible with 0.11.0-dev.1912 (including supporting the new built-in modules)...

BUT...0.11-dev has no support for async, so the code will now fallback to threads. Also, 0.11-dev is constantly changing. I'll try to stay up to date with it, but there'll almost certainly be more incompatibility as 0.11-dev continues to evolve.