RedisDataDecoder#parseBulkString Array index is out of range

[mandarin6b0]

https://github.com/vapor/redis/blob/b4518963bfdc6707378741ad999e629e880b6128/Sources/Redis/Coders/RedisDataDecoder.swift#L166-L169

Stumbled upon the crash today with slicing bytes array with size out of range. The state at that moment was:

size == 10
buffer.readableBytes == 115
position == 113
bytes: 2 elements

and the buffer.readString(length: 115) output

"*8\r\n$16\r\ntest8:1523640910\r\n$10\r\n1523640910\r\n$16\r\ntest9:1523640913\r\n$10\r\n1523640913\r\n$17\r\ntest10:1523640916\r\n$10\r\n15"

Is out of range expected behaviour with that buffer?

[mandarin6b0]

I expected from redis

1) "test8:1523640910"
2) "1523640910"
3) "test9:1523640913"
4) "1523640913"
5) "test10:1523640916"
6) "1523640916"
7) "test11:1523640918"
8) "1523640918"

So it seems that buffer either is broken or is not fully parsed?

[pedantix]

that should not be the behavior the size should be contrained to the size of the buffer. I will add a test case with that string and see if I can test it out

[pedantix]

I was able to modify the guard relatively easy, sorry the test suite for the parser didn't have this partial case tested out. PR to fix https://github.com/vapor/redis/pull/102

[pedantix]

you can use my branch to keep going while we wait on @tanner0101 to review this.... he has a ton going on and will probably not get to it I imagine until my PR to create a KeyedCacheSessionStore goes in

[pedantix]

I sometimes wonder if due to the nature of partial strings in redis RESP I should have used a recursive decoder pattern that just throws an exception on partial and gets caught at the top of the decode context .... I don't think this would be as performant but may have been cleaner and less error prone. If this becomes a reoccurence I will consider moving to it

[mandarin6b0]

I'm not confident enough in my knowledge of how decoding is working now, so can't say anything specific on that matter :). A bit of offtop, didn't found related docs about this matter. Does RedisClient#command supposed to be called from one worker only? I've got swift retain crush that I could not recreate at this moment that fired around redis decoding. And guessing where the problem could be.

[pedantix]

RedisClient should only be called on the EventLoop that created it. This has to do with SwiftNIO

[pedantix]

Sorry it is a limitation of a system like NIO but you should be able to develop a pattern in both HTTP/1 and WebSockets that works well and cleanly. I think I have example apps of both at the moment

[mandarin6b0]

Oh, I totally thought that it will do syncing itself internaly because the worker was passed as argument at client connect stage. No, calling from one eventLoop is not a problem at all for me. Just didn't expected. Well, maybe I didn't read docs properly :)

[pedantix]

Our docs really aren't up to date, not your fault other then we should all assume things change rapidly while we are still in beta. I think it would probably be a worthwhile thing writing up the pros and cons of EventLoops and Channel handlers. Essentially they are very fast and very performant but we have to make async calls from inside the same event loop that connections are established on. There really isn't a lot of mystery beyond that just the way NIO works.