[asterisk-dev] [Code Review] [branch] Expand select(2) bits to allow for more than FD_SETSIZE file descriptors

[Tilghman Lesher]

> On 2010-08-11 13:14:10, Russell Bryant wrote:
> > /branches/1.4/main/poll.c, lines 322-326
> > <https://reviewboard.asterisk.org/r/824/diff/3/?file=12169#file12169line322>
> >
> >     So I guess this is safe because we have ensured that ast_fdset can support the maximum number of file descriptors Asterisk is allowed to open?

There's no way to ensure that, given that it's a declared type.  However, by using the replaced versions of FD_SET, we will complain and refuse to corrupt beyond the length of the bitfield.


> On 2010-08-11 13:14:10, Russell Bryant wrote:
> > /branches/1.4/res/res_features.c, line 2319
> > <https://reviewboard.asterisk.org/r/824/diff/3/?file=12170#file12170line2319>
> >
> >     check for allocation failure

Changed.


> On 2010-08-11 13:14:10, Russell Bryant wrote:
> > /branches/1.4/res/res_features.c, lines 2368-2370
> > <https://reviewboard.asterisk.org/r/824/diff/3/?file=12170#file12170line2368>
> >
> >     This is probably an undesirable change, as it's just formatting and will result in a conflict when you merge it up.

Possibly, but the red spot bugged me.


> On 2010-08-11 13:14:10, Russell Bryant wrote:
> > /branches/1.4/res/res_features.c, lines 2484-2487
> > <https://reviewboard.asterisk.org/r/824/diff/3/?file=12170#file12170line2484>
> >
> >     Should revents get reset here, too?

Changed.


> On 2010-08-11 13:14:10, Russell Bryant wrote:
> > /branches/1.4/channels/chan_misdn.c, line 4917
> > <https://reviewboard.asterisk.org/r/824/diff/3/?file=12159#file12159line4917>
> >
> >     the use of named initializers would be a nice little improvement here and in similar places:
> >     
> >     struct pollfd pfd = {
> >         .fd = ch->pipe[1],
> >         .events = POLLOUT,
> >     };

Added.


> On 2010-08-11 13:14:10, Russell Bryant wrote:
> > /branches/1.4/channels/chan_alsa.c, lines 284-307
> > <https://reviewboard.asterisk.org/r/824/diff/3/?file=12158#file12158line284>
> >
> >     Does the revents field need to be reset on each iteration of this loop?
> 
> Kevin Fleming wrote:
>     Yes it does. poll() does not initialize the revents fields of the array itself.

Changed.


- Tilghman


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/824/#review2556
-----------------------------------------------------------


On 2010-08-11 14:41:05, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/824/
> -----------------------------------------------------------
> 
> (Updated 2010-08-11 14:41:05)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Presently, when the number of file descriptors expands beyond FD_SETSIZE descriptors, we have troubles when those file descriptors are used with select(2).  Usually, this causes memory corruption, because the bitfields are not large enough, and parts of memory beyond the bitfield are corrupted.  This change both expands the default bitfield, as well as adding checks to ensure that memory beyond the end of bitfields are not corrupted.
> 
> 
> This addresses bug 17678.
>     https://issues.asterisk.org/view.php?id=17678
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_alsa.c 281727 
>   /branches/1.4/channels/chan_misdn.c 281727 
>   /branches/1.4/channels/chan_oss.c 281727 
>   /branches/1.4/channels/chan_phone.c 281727 
>   /branches/1.4/configure UNKNOWN 
>   /branches/1.4/configure.ac 281727 
>   /branches/1.4/include/asterisk/autoconfig.h.in 281727 
>   /branches/1.4/include/asterisk/channel.h 281727 
>   /branches/1.4/include/asterisk/poll-compat.h 281727 
>   /branches/1.4/include/asterisk/select.h PRE-CREATION 
>   /branches/1.4/main/asterisk.c 281727 
>   /branches/1.4/main/poll.c 281727 
>   /branches/1.4/res/res_features.c 281727 
> 
> Diff: https://reviewboard.asterisk.org/r/824/diff
> 
> 
> Testing
> -------
> 
> Architectural review only.  Testing to commence, when other developers agree with the architecture of these changes.
> 
> 
> Thanks,
> 
> Tilghman
> 
>