The Inbox: Network-tonyg.186.mcz

> A new version of Network was added to project The Inbox:
> http://source.squeak.org/inbox/Network-tonyg.186.mcz
>
> ==================== Summary ====================
>
> Name: Network-tonyg.186
> Author: tonyg
> Time: 19 February 2017, 12:00:54.097194 pm
> UUID: e9ef40b8-5a51-460a-b2cf-54d43fca0b9c
> Ancestors: Network-tfel.185
>
> Clamp start position in search in SocketStream>>upToAll: to avoid out-of-bounds access when searching in binary mode. Proposed fix for problems highlighted by tests included in NetworkTests-tonyg.44.
>
> =============== Diff against Network-tfel.185 ===============
>
> Item was changed:
>   ----- Method: SocketStream>>upToAll:limit: (in category 'stream in') -----
>   upToAll: aStringOrByteArray limit: nBytes
>   "Answer a subcollection from the current access position to the occurrence (if any, but not inclusive) of aStringOrByteArray. If aCollection is not in the stream, or not found within nBytes answer the available contents of the stream"
>  
>   | index sz result searchedSoFar target |
>   "Deal with ascii vs. binary"
>   self isBinary
>   ifTrue:[target := aStringOrByteArray asByteArray]
>   ifFalse:[target := aStringOrByteArray asString].
>  
>   sz := target size.
>   "Look in the current inBuffer first"
>   index := inBuffer indexOfSubCollection: target
> + startingAt: ((lastRead - sz + 2) max: 1).
> - startingAt: lastRead - sz + 2.
>   (index > 0 and: [(index + sz) <= inNextToWrite]) ifTrue: ["found it"
>   result := self nextInBuffer: index - lastRead - 1.
>   self skip: sz.
>   ^ result
>   ].
>  
>   [searchedSoFar :=  self inBufferSize.
>   "Receive more data"
>   self receiveData.
>   recentlyRead > 0] whileTrue:[
>  
>   "Data begins at lastRead + 1, we add searchedSoFar as offset and
>   backs up sz - 1 so that we can catch any borderline hits."
>  
>   index := inBuffer indexOfSubCollection: target
>   startingAt: (lastRead + searchedSoFar - sz + 2 max: 1).
>   (index > 0 and: [(index + sz) <= inNextToWrite]) ifTrue: ["found it"
>   result := self nextInBuffer: index - lastRead - 1.
>   self skip: sz.
>   ^ result
>   ].
>   "Check if we've exceeded the max. amount"
>   (nBytes notNil and:[inNextToWrite - lastRead > nBytes])
>   ifTrue:[^self nextAllInBuffer].
>   ].
>  
>   "not found and (non-signaling) connection was closed"
>   ^self nextAllInBuffer!
>
>

> What do people think of this proposed fix?
>
> I think it, at worst, does no harm, and makes the tests I added in
> NetworkTests-tonyg.44 pass.
>
> Tony
>
> PS. Using the 6.0alpha has been really, really pleasant. Kudos to all
> who have made it such a great experience.
>
>
>
> On 02/19/2017 05:01 PM, [hidden email] wrote:
>> A new version of Network was added to project The Inbox:
>> http://source.squeak.org/inbox/Network-tonyg.186.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Network-tonyg.186
>> Author: tonyg
>> Time: 19 February 2017, 12:00:54.097194 pm
>> UUID: e9ef40b8-5a51-460a-b2cf-54d43fca0b9c
>> Ancestors: Network-tfel.185
>>
>> Clamp start position in search in SocketStream>>upToAll: to avoid out-of-bounds access when searching in binary mode. Proposed fix for problems highlighted by tests included in NetworkTests-tonyg.44.
>>
>> =============== Diff against Network-tfel.185 ===============
>>
>> Item was changed:
>>   ----- Method: SocketStream>>upToAll:limit: (in category 'stream in') -----
>>   upToAll: aStringOrByteArray limit: nBytes
>>   "Answer a subcollection from the current access position to the occurrence (if any, but not inclusive) of aStringOrByteArray. If aCollection is not in the stream, or not found within nBytes answer the available contents of the stream"
>>
>>   | index sz result searchedSoFar target |
>>   "Deal with ascii vs. binary"
>>   self isBinary
>>   ifTrue:[target := aStringOrByteArray asByteArray]
>>   ifFalse:[target := aStringOrByteArray asString].
>>
>>   sz := target size.
>>   "Look in the current inBuffer first"
>>   index := inBuffer indexOfSubCollection: target
>> + startingAt: ((lastRead - sz + 2) max: 1).
>> - startingAt: lastRead - sz + 2.
>>   (index > 0 and: [(index + sz) <= inNextToWrite]) ifTrue: ["found it"
>>   result := self nextInBuffer: index - lastRead - 1.
>>   self skip: sz.
>>   ^ result
>>   ].
>>
>>   [searchedSoFar :=  self inBufferSize.
>>   "Receive more data"
>>   self receiveData.
>>   recentlyRead > 0] whileTrue:[
>>
>>   "Data begins at lastRead + 1, we add searchedSoFar as offset and
>>   backs up sz - 1 so that we can catch any borderline hits."
>>
>>   index := inBuffer indexOfSubCollection: target
>>   startingAt: (lastRead + searchedSoFar - sz + 2 max: 1).
>>   (index > 0 and: [(index + sz) <= inNextToWrite]) ifTrue: ["found it"
>>   result := self nextInBuffer: index - lastRead - 1.
>>   self skip: sz.
>>   ^ result
>>   ].
>>   "Check if we've exceeded the max. amount"
>>   (nBytes notNil and:[inNextToWrite - lastRead > nBytes])
>>   ifTrue:[^self nextAllInBuffer].
>>   ].
>>
>>   "not found and (non-signaling) connection was closed"
>>   ^self nextAllInBuffer!
>>
>>

> Hi Levente,
>
> On 02/23/2017 01:48 PM, Levente Uzonyi wrote:
>> I'd rather see the implementation of SequenceableCollection >>
>> #indexOfSubCollection:startingAt:ifAbsent: changed to accept the invalid
>> indices and just evaluate exceptionBlock when the needle was not found -
>> the same way String's implementation behaves.
>
> Thanks for the review!
>
> I've uploaded:
> - CollectionsTests-tonyg.275, which adds some tests for the method
> - Collections-tonyg.733.1 (sorry for the branch, avoids conflict with
>   the recent .734 of mine), which fixes the tests and also fixes the
>   tests that were added in NetworkTests-tonyg.44
>
> I didn't make the other changes you suggest, because I wanted to limit
> the scope of the changes to just the bug fixes for now.
>
> Do you think the proposed changes are acceptable now?
>
> Regards,
>  Tony
>