Smalltalk › Squeak › Squeak - Dev

Flaw in SocketStream>>peek detected

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

8 messages Options

Igor Stasenko

Flaw in SocketStream>>peek detected

Peek should answer a next item to read , but do not advance the stream position.
Therefore a following test should yield true:

a := anyStream peek.
b := anyStream next.

self assert: (a = b).

In a SocketStream, however , #next, first advances the lastRead ivar,
then reads a next char
answering inBuffer at: pos+1 ( where pos is the lastRead value
before sending #next )

SocketStream>>next
"Return next byte, if inBuffer is empty
we recieve some more data and try again."

self atEnd ifTrue: [^nil].
self isInBufferEmpty ifTrue:
[self receiveData.
self atEnd ifTrue: [^nil]].
lastRead := lastRead + 1.
^inBuffer at: lastRead

But peek, answering a char from inBuffer at: pos !!!

peek
"Return next byte, if inBuffer is empty
we recieve some more data and try again.
Do not consume the byte."

self atEnd ifTrue: [^nil].
self isInBufferEmpty ifTrue:
[self receiveData.
self atEnd ifTrue: [^nil]].
^inBuffer at: lastRead

The fix is:

--- ^inBuffer at: lastRead
+++ ^inBuffer at: lastRead+1

but i'm not sure if something else won't break because of it ;)

--
Best regards,
Igor Stasenko AKA sig.

Igor Stasenko

Re: Flaw in SocketStream>>peek detected

http://bugs.squeak.org/view.php?id=7446

--
Best regards,
Igor Stasenko AKA sig.

Stéphane Ducasse

Re: [Pharo-project] Flaw in SocketStream>>peek detected

http://code.google.com/p/pharo/issues/detail?id=1813

On Jan 13, 2010, at 8:49 PM, Igor Stasenko wrote:

> http://bugs.squeak.org/view.php?id=7446
>
> --
> Best regards,
> Igor Stasenko AKA sig.
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

Andreas.Raab

Re: Flaw in SocketStream>>peek detected

In reply to this post by Igor Stasenko

Igor Stasenko wrote:
> The fix is:
>
> --- ^inBuffer at: lastRead
> +++ ^inBuffer at: lastRead+1
>
>
> but i'm not sure if something else won't break because of it ;)

Extremely unlikely. It's pretty clear that peek is wrong here as
illustrated by:

stream := SocketStream openConnectionToHostNamed: 'www.google.com' port: 80.
stream nextPutAll:('GET / HTTP/1.0\\' copyReplaceAll: '\'with: String crlf).
stream flush.
stream peek.

This blows up and clearly it shouldn't.

Cheers,
- Andreas

johnmci

Re: Re: Flaw in SocketStream>>peek detected

Er, well also it's a timing issue just because you did the nextPutAll: and the flush.

(a) doesn't mean the GET/ actually got to the receiver, but it's in the network stack somewhere.
(b) stream peek will it return data, what if www.google.com never answers and closes the socket?

"Receive data into the given buffer and return the number of bytes received.
Note the given buffer may be only partially filled by the received data.
Waits for data once. The answer may be zero (indicating that no data was
available before the socket closed)."

On 2010-01-13, at 10:29 PM, Andreas Raab wrote:

> Igor Stasenko wrote:
>> The fix is:
>> --- ^inBuffer at: lastRead
>> +++ ^inBuffer at: lastRead+1
>> but i'm not sure if something else won't break because of it ;)
>
> Extremely unlikely. It's pretty clear that peek is wrong here as illustrated by:
>
> stream := SocketStream openConnectionToHostNamed: 'www.google.com' port: 80.
> stream nextPutAll:('GET / HTTP/1.0\\' copyReplaceAll: '\'with: String crlf).
> stream flush.
> stream peek.
>
> This blows up and clearly it shouldn't.
>
> Cheers,
> - Andreas
>

--
===========================================================================
John M. McIntosh <[hidden email]> Twitter: squeaker68882
Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com
===========================================================================

Andreas.Raab

Re: Flaw in SocketStream>>peek detected

John M McIntosh wrote:
> Er, well also it's a timing issue just because you did the nextPutAll: and the flush.

It's not a timing issue. Please run the code - it blows up after the
response is received with an index out of bounds: 0 because it's trying
to access lastRead and lastRead is zero at this point (nothing has been
read thus far). No timing issues involved.

Cheers,
- Andreas

>
> (a) doesn't mean the GET/ actually got to the receiver, but it's in the network stack somewhere.
> (b) stream peek will it return data, what if www.google.com never answers and closes the socket?
>
> "Receive data into the given buffer and return the number of bytes received.
> Note the given buffer may be only partially filled by the received data.
> Waits for data once. The answer may be zero (indicating that no data was
> available before the socket closed)."
>
>
>
> On 2010-01-13, at 10:29 PM, Andreas Raab wrote:
>
>> Igor Stasenko wrote:
>>> The fix is:
>>> --- ^inBuffer at: lastRead
>>> +++ ^inBuffer at: lastRead+1
>>> but i'm not sure if something else won't break because of it ;)
>> Extremely unlikely. It's pretty clear that peek is wrong here as illustrated by:
>>
>> stream := SocketStream openConnectionToHostNamed: 'www.google.com' port: 80.
>> stream nextPutAll:('GET / HTTP/1.0\\' copyReplaceAll: '\'with: String crlf).
>> stream flush.
>> stream peek.
>>
>> This blows up and clearly it shouldn't.
>>
>> Cheers,
>> - Andreas
>>
>
> --
> ===========================================================================
> John M. McIntosh <[hidden email]> Twitter: squeaker68882
> Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com
> ===========================================================================
>
>
>
>
>
>

Göran Krampe

Re: Re: Flaw in SocketStream>>peek detected

In reply to this post by Andreas.Raab

Hi all!

I presume this bug is also in Pharo so I CCed.

Andreas Raab wrote:

> Igor Stasenko wrote:
>> The fix is:
>>
>> --- ^inBuffer at: lastRead
>> +++ ^inBuffer at: lastRead+1
>>
>>
>> but i'm not sure if something else won't break because of it ;)
>
> Extremely unlikely. It's pretty clear that peek is wrong here as
> illustrated by:
>
> stream := SocketStream openConnectionToHostNamed: 'www.google.com' port:
> 80.
> stream nextPutAll:('GET / HTTP/1.0\\' copyReplaceAll: '\'with: String
> crlf).
> stream flush.
> stream peek.
>
> This blows up and clearly it shouldn't.

Yup, just fix it! :) Sorry for the bug guys.

regards, Göran

johnmci

Re: Flaw in SocketStream>>peek detected

In reply to this post by Andreas.Raab

Yes I dd run the code and it doesn't handle the edge case of what happens when no-one before has read any bytes.
I was just pointing out the fact that peeking on a socket is complicated and one needs to think more carefully about all possible outcomes
On 2010-01-13, at 11:06 PM, Andreas Raab wrote:

> John M McIntosh wrote:
>> Er, well also it's a timing issue just because you did the nextPutAll: and the flush.
>
> It's not a timing issue. Please run the code - it blows up after the response is received with an index out of bounds: 0 because it's trying to access lastRead and lastRead is zero at this point (nothing has been read thus far). No timing issues involved.
>
> Cheers,
> - Andreas

--
===========================================================================
John M. McIntosh <[hidden email]> Twitter: squeaker68882
Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com
===========================================================================