Error in clearing write buffer bubbles up as an array IndexError

[niceking]

Hello, I've got a websocket implementation in a gem, and occasionally on a write I'm seeing a IndexError: negative length -1?

It seems that the nwrote variable in this method is set to -1 somehow and it's bubbling up as an array error.

I'm on Ruby 2.7.2 and OpenSSL library version 2.1.2 (not quite sure how that version number works as that nwrote var was introduced in 2.2.0 🤷‍♀️)

[rhenium]

Is SSLSocket#write (or #flush, #print, etc.) called from multiple threads? Please note that the buffered IO is currently not thread-safe.

However, the way it breaks is unexpected -- SSLSocket#syswrite should never return a negative number. This seems to be because #syswrite isn't properly protecting the string buffer from being modified (emptied) by another thread while waiting for the underlying socket.

[niceking]

Oh ok I didn't realise about the thread safety. I think there is a possible code path in my gem's implementation which would mean that SSLSocket#write is called from multiple threads, I'll do some debugging and see if I can address it on my end.

In the mean time do you think there'll be a fix for SSLSocket#syswrite returning a negative number?

[niceking]

Hmm I did some poking around I don't think there's anywhere in the gem that could be calling write from multiple threads. We are using the same socket in multiple threads, but one only reads from it and the other only writes from it. Is there any other scenario that could trigger that -1 being returned?

[ioquatix]

OpenSSL can read and write from the underlying socket even for only "read" or "write" operations separately. So socket.read might actually write to the socket (at least in my understanding).

Therefore, I don't believe it's safe to read and write from separate threads, at least in this case.

You could confirm this by adding a Thread::Mutex around the read and write operations to guard the socket.

[rhenium]

#453 should fix the -1 return from #syswrite (and #sysread which had a similar problem).

[niceking]

Hey @rhenium thanks for the fix! I read through your PR but my 2 years as a C developer 10 years ago wasn't much help for understanding it 😛

I'm wondering what the change means in the context of the error I experienced? Given that the string buffer will be frozen, if another thread attempts to write to the same socket, will it block and wait? Or will the information it is attempting to write be discarded? Or will it throw a different type of error? And does this bring the SSL Socket read and write ops closer to being thread safe?

I'm a little surprised by your comment @ioquatix that reading and writing from the same socket isn't thread safe. I'm using readpartial not read but I assume your comment still applies? Like given the underlying socket technology is full duplex, and given the asynchronous nature of data being streamed, feels like having one thread to handle reading and another to handle writing to the same socket would be a pretty common use case to support??

[ioquatix]

There is no guarantee that reading and writing to the same OpenSSL socket is thread safe. Sure, the underling OS socket might be... but that's not what you are reading and writing from here.

[ioquatix]

reading and writing from the same socket isn't thread safe

I'm not sure anyone could make this guarantee at the Ruby level. There are a lot of edge cases in Ruby's IO layer that the GVL usually protects against but I've personally noticed some nasty edge cases.

[niceking]

Yeah fair enough! We have built some retry/confirm stuff as part of this so probably better to ensure that works better than relying on writes to always be successful

[ioquatix]

If you want something which is definitely safe and predictable, you could use async. You can embed async into other programs, but you can't share the same socket between threads (same issues apply). But you can read and write to the same socket using two fibers which is 100% safe.

[JuanitoFatas]

@rhenium Thanks so much for fixing!! Which version of OpenSSL gem could we use your fix? 😊 Is it possible to release a new version?

[rhenium]

I pushed openssl gem v2.1.3 and v2.2.1 a few days ago, which contain #453. (changelog: https://github.com/ruby/openssl/blob/2b3b29b973af9ae2433aca6f9a0a7653a48434c2/History.md)

[rhenium]

IndexError: negative length -1 error will not appear in v2.1.3/v2.2.1 anymore.

Note that, however, the error showing up means your code is writing to an SSLSocket from multiple threads. This is not a supported operation and you should fix it.