http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #9 from felipe.contreras@gmail.com  2007-07-15 17:25 -------
(In reply to comment #6)
> (In reply to comment #5)
> > Also the format of all the source files seems to be wrong.
> > 
> 
> As I recall, Walter uses the strange imho convention of 4 spaces to an indent, but 8 spaces to a tab.  Since many editors for any graphical interface use 4 spaces to a tab now, this can cause some confusion.
> 
> But don't worry, they are formatted properly when viewed through the right glasses.
> 
> -[Unknown]
> 

I mean, I get a ^M at the end of each line when I try to view the files in vim, Linux.

I did try to change the file format to dos, but it still looks the same.


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #10 from felipe.contreras@gmail.com  2007-07-15 17:56 -------
(In reply to comment #7)
> The whole SocketSet class looks rather odd to me (given only a few minutes looking at it).  It's confusing 'max' as a number of sockets that can be added and the fact that the socket fd value won't necessarily have any relation to that count.  Other oddities are functions like this:
> 
> socket_t* first() { return cast(socket_t*)buf; }
> fd_set* _fd_set() { return cast(fd_set*)buf; }
> 
> Treating the same block of memory as two different and incompatible things. Does this class even work as it's name suggests?
> 
> The proposed change to the constructor doesn't feel right given the arbitrary value that a socket fd might have.  Using anything other than FD_SETSIZE for 'max' is likely to result in problems.  NFDBITS is simply the number of bits that fit in a word which doesn't make much sense as a minimum for nbytes.
> 
> Really, I'd just ditch the entire concept of a variable size set.  It doesn't make any sense for unix style select() api's.  For other type of selection interfaces it might make more sense such as epoll on linux, but there the actual set is managed inside the kernel, not user space.
> 

I agree, max should always be FD_SETSIZE.

/* Maximum number of file descriptors in `fd_set'.  */ #define FD_SETSIZE              __FD_SETSIZE


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #11 from braddr@puremagic.com  2007-07-15 19:18 -------
Created an attachment (id=152)
 --> (http://d.puremagic.com/issues/attachment.cgi?id=152&action=view)
Major overhaul to SocketSet -- untested

Walter, here's a potential rewrite of SocketSet but it's untested in it's entirety.  The only thing I confirmed is that it compiles with 1.018 on linux. I haven't test compiled win32.  I haven't tried it with any test apps. Unfortunately there's no unittests to make life easy.


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268


braddr@puremagic.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #152 is|0                           |1
              patch|                            |




-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #12 from felipe.contreras@gmail.com  2007-07-15 19:29 -------
(In reply to comment #10)
> (In reply to comment #7)
> > The whole SocketSet class looks rather odd to me (given only a few minutes looking at it).  It's confusing 'max' as a number of sockets that can be added and the fact that the socket fd value won't necessarily have any relation to that count.  Other oddities are functions like this:
> > 
> > socket_t* first() { return cast(socket_t*)buf; }
> > fd_set* _fd_set() { return cast(fd_set*)buf; }
> > 
> > Treating the same block of memory as two different and incompatible things. Does this class even work as it's name suggests?
> > 
> > The proposed change to the constructor doesn't feel right given the arbitrary value that a socket fd might have.  Using anything other than FD_SETSIZE for 'max' is likely to result in problems.  NFDBITS is simply the number of bits that fit in a word which doesn't make much sense as a minimum for nbytes.
> > 
> > Really, I'd just ditch the entire concept of a variable size set.  It doesn't make any sense for unix style select() api's.  For other type of selection interfaces it might make more sense such as epoll on linux, but there the actual set is managed inside the kernel, not user space.
> > 
> 
> I agree, max should always be FD_SETSIZE.
> 
> /* Maximum number of file descriptors in `fd_set'.  */ #define FD_SETSIZE              __FD_SETSIZE
> 

(In reply to comment #7)
> The whole SocketSet class looks rather odd to me (given only a few minutes looking at it).  It's confusing 'max' as a number of sockets that can be added and the fact that the socket fd value won't necessarily have any relation to that count.  Other oddities are functions like this:
> 
> socket_t* first() { return cast(socket_t*)buf; }
> fd_set* _fd_set() { return cast(fd_set*)buf; }
> 
> Treating the same block of memory as two different and incompatible things. Does this class even work as it's name suggests?
> 
> The proposed change to the constructor doesn't feel right given the arbitrary value that a socket fd might have.  Using anything other than FD_SETSIZE for 'max' is likely to result in problems.  NFDBITS is simply the number of bits that fit in a word which doesn't make much sense as a minimum for nbytes.
> 
> Really, I'd just ditch the entire concept of a variable size set.  It doesn't make any sense for unix style select() api's.  For other type of selection interfaces it might make more sense such as epoll on linux, but there the actual set is managed inside the kernel, not user space.
> 

I did some research.

The real problem is that FD_ZERO always erases the whole fd_set. Since the fd_set didn't have the right size then more memory was erased and the corruption generated the crash.

So we _need_ to have fd_set width the right size which turns out to be: FD_SETSIZE / NFDBITS * int.size

That's because inside fd_set there isn't a list of sockets, but a binary map. So if bit 0 is on, that means the socket with fd 0 should be checked and so on. So the first() method was wrong, there's no way from the fd_set to find the socket_t structure.

I'm not sure why a size of NFDBITS makes it not crash, maybe the zeroed structures are not used in this case.

Anyway, I have tested the attached code with up to 1024 sockets. The assert doesn't get activated because the socket creation fails.


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #13 from felipe.contreras@gmail.com  2007-07-15 19:30 -------
Created an attachment (id=153)
 --> (http://d.puremagic.com/issues/attachment.cgi?id=153&action=view)
Proposed patch v2.0


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #14 from braddr@puremagic.com  2007-07-15 23:59 -------
For what it's worth, I just tested my rewrite with the Test file that Felipe provided.

Felipe, would you please apply my changes, a more thorough fix / rewrite than your changes, and see how they do for you?


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268


braddr@puremagic.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #152 is|0                           |1
           obsolete|                            |




------- Comment #15 from braddr@puremagic.com  2007-07-16 00:08 -------
Created an attachment (id=154)
 --> (http://d.puremagic.com/issues/attachment.cgi?id=154&action=view)
Major overhaul to SocketSet -- lightly tested -- v2

Missed decrementing SocketSet.count during SocketSet.remove


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #16 from bugzilla@digitalmars.com  2007-07-16 01:50 -------
Can you please expand the test case so it at least gives good coverage via the -cov switch? This will save us a lot of trouble.


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=268





------- Comment #17 from vietor@zettabytestorage.com  2007-07-19 19:09 -------
As an additional data point, I would like to confirm that "Proposed patch v2.0" fixes some memory corruption I was experiencing when using SocketSets with a small (4) max number of sockets.

The ^M pollution is due to inconsistent line termination standards which should
have been resolved 15 years ago. You can strip them pretty easily with:
tr -d "\015" < infile.d

Sadly that's not going to get you very far as it'll then be quite inconsistent with the rest of the source. Not that I would mind seeing them vanish from the entirety of the source, but that's a different issue ...


--