Issue 5423 in pharo: Prevent accidental semaphore leaks when using Socket acceptFrom

Status: Accepted
Owner: [hidden email]
Labels: Milestone-1.4

New issue 5423 by [hidden email]: Prevent accidental semaphore leaks when  
using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

Socket >> acceptFrom: is a dangerous method, since it unconditionally  
replaces existing handles/semaphores in a Socket instance.

It should be bulletproofed to avoid user error, either by:
1) having it unregister existing handle/semaphores (The "I hope you weren't  
using that!" approach), or
2) raise an error in that case, notifying the user he is trying to accept  
using an already initialized socket. (The "Surely you jest, madame?"  
approach)

Personally I favor 2, since it is more explicit that using an existing  
Socket is a source of error, and you must be sure you are not using it for  
anything else anymore to avoid suble errors.

This is not a regression per se, but it is an important fix for ensuring a  
by default stable environment.

The inspiration is RFB, where a Socket subclass accidentally called super  
new acceptFrom:, and leaked semaphores due to Socket new returning an  
initialized TCP socket.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #1 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

i vote for 2.

it resonates with socket library design: once you bound socket,
attempts to (re)bind it again leading to error:

EINVAL

The socket is already bound to an address.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #2 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

Yes, option 2 seems best.

Great catch! Thanks again for tracking this down in RFB.

 From complaints on the mailing lists I long suspected RFB to play a role in  
problems with long running server images. I never used it.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: Type-Bug

Comment #3 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: -Milestone-1.4

Comment #4 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

I will remove the 1.4 tag from this one (as it is a problem in 1.3 already  
and the world did not stop to turn)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #5 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

good idea



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: Milestone-3.0

Comment #6 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

ok, I want this in 3.0, is just too important (RFB is not working propwrly  
in pharo, and it hangs the image time to time)

check this:
http://forum.world.st/leaking-semaphores-td4432983.html#a4433697


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #7 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

Since RFB  has quite a few users, I guess the real question is;
Do we wait for/ prod its package maintainers to release the fix before  
doing this, or introduce the error message now, breaking RFB (as in, not  
working at all instead of eventually causing the image to hang due to  
leaked semaphores) in the process, and hope that encourages maintainers to  
respond?

The change itself is fairly simple, along the lines of:

Socket >> #acceptFrom: aSocket
        "Initialize a new socket handle from an accept call"

        | semaIndex readSemaIndex writeSemaIndex |
        socketHandle ifNotNil: [^Error signal: 'The socket is already bound'].
        semaphore := Semaphore new.

(and equivalent for #initialize: to be consistent)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #8 on issue 5423 by [hidden email]: Prevent accidental  
semaphore leaks when using Socket acceptFrom
http://code.google.com/p/pharo/issues/detail?id=5423

Euh, the fix is so simple, why not do it in 2.0 ??


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker