[PATCH rtems-libbsd] ipsec-tools: Fix copying fd_set prior to select

[Chris Johns]

On 24/5/2023 9:13 am, Chris Johns wrote:
> On 23/5/2023 5:30 pm, Christian MAUDERER wrote:
>> Hello Chris,
>>
>> On 2023-05-23 08:53, Chris Johns wrote:
>>> On 23/5/2023 4:25 pm, Christian MAUDERER wrote:
>>>> Hello Chris,
>>>>
>>>> On 2023-05-23 03:36, Chris Johns wrote:
>>>>> Hi,
>>>>>
>>>>> I have been resolving this by adding:
>>>>>
>>>>> #define preset_mask *preset_mask_prealloc
>>>>> #define active_mask *active_mask_prealloc
>>>>>
>>>>> as the assignment logic works as is. It does not catch FD_ZERO which is a shame
>>>>> but is helps avoid us adding a special case.
>>>>
>>>> Thanks for the suggestion. I used that solution already in the file during the
>>>> initial port. But it doesn't work in this case (and maybe we should check other
>>>> cases in libbsd too). It only copies the first element of the array of fd_sets.
>>>>
>>>> The variables / defines are (in the patched version - see [1]):
>>>>
>>>> static fd_set *allocated_preset_mask, *allocated_active_mask;
>>>> static struct fd_monitor *allocated_fd_monitors;
>>>> #define preset_mask (*allocated_preset_mask)
>>>> #define active_mask (*allocated_active_mask)
>>>> #define fd_monitors (allocated_fd_monitors)
>>>>
>>>> They are allocated as follows (see [2]):
>>>>
>>>>      allocated_preset_mask = calloc(sizeof(fd_set),
>>>>          howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>>>      allocated_active_mask = calloc(sizeof(fd_set),
>>>>          howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>>>      allocated_fd_monitors = calloc(
>>>>          rtems_libio_number_iops, sizeof(struct fd_monitor));
>>>>
>>>> So basically it's a bunch of arrays of fd_sets. If I now use the following
>>>> (unpatched) assignment:
>>>>
>>>>      active_mask = preset_mask;
>>>>
>>>> After the preprocessor the line will be
>>>>
>>>>      (*allocated_active_mask) = (*allocated_preset_mask);
>>>>
>>>> Which copies only the first element. That's why I had to add the memcpy. Did I
>>>> miss some better solution?
>>>
>>> I am using this in ntpd which I checked yesterday:
>>>
>>> #ifdef __rtems__
>>> #include <rtems/libio_.h>
>>> static size_t rtems_ntpd_fds_size;
>>> static int rtems_fd_set_alloc(fd_set **setp) {
>>>          if (*setp == NULL) {
>>>                  rtems_ntpd_fds_size = sizeof(fd_set) *
>>> (howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>>                  *setp = malloc(rtems_ntpd_fds_size);
>>>                  if (*setp == NULL) {
>>>                          errno = ENOMEM;
>>>                          return -1;
>>>                  }
>>>                  memset(*setp, 0, rtems_ntpd_fds_size);
>>>          }
>>>          return 0;
>>> }
>>> #define activefds (*activefds_prealloc)
>>> static fd_set *activefds_prealloc;
>>> #define rtems_activefds_alloc() rtems_fd_set_alloc(&activefds_prealloc)
>>> #else /* __rtems__ */
>>> static fd_set activefds;
>>> #endif /* __rtems__ */
>>>
>>> and:
>>>
>>> #if __rtems__
>>>          #define rdfdes (*rdfdes_prealloc)
>>>          static fd_set *rdfdes_prealloc;
>>> #else /* __rtems__ */
>>>          fd_set rdfdes;
>>> #endif /* __rtems__ */
>>>          int nfound;
>>>
>>> #if __rtems__
>>>          if (rtems_fd_set_alloc(&rdfdes_prealloc) < 0) {
>>>                  return;
>>>          }
>>>          memset(rdfdes_prealloc, 0, rtems_ntpd_fds_size);
>>> #else /* __rtems__ */
>>>          FD_ZERO(&set);
>>> #endif /* __rtems__ */
>>>
>>> with:
>>>
>>>          ++handler_calls;
>>>          rdfdes = activefds;
>>
>> But isn't that line resolving to
>>
>>   (*rdfdes_prealloc) = (*activefds_prealloc);
>>
>> That would also copy only the first element.
>>
>>>
>>> Stepping:
>>>
>>> Thread 9 hit Breakpoint 1, _ntp_io_handler () at
>>> ../../bsd/freebsd/contrib/ntp/ntpd/ntp_io.c:3727
>>> 3727            rdfdes = activefds;
>>> (gdb) x /8bx activefds_prealloc
>>> 0x1083e650:     0x20    0x44    0x05    0x20    0x00    0x40    0x00    0x00
>>> (gdb) x /8bx rdfdes_prealloc
>>> 0x108353a0:     0x00    0x00    0x00    0x00    0x00    0x00    0x00    0x00
>>> (gdb) n
>>> 3736                    t1.tv_usec = 0;
>>> (gdb) x /8bx rdfdes_prealloc
>>> 0x108353a0:     0x20    0x44    0x05    0x20    0x00    0x40    0x00    0x00
>>>
>>> Looks ok to me.
>>
>> fd_set is 64 bit wide. So a copy of the first 64 bit works. Does it still work
>> if you have 256 files in the system?
> 
> Good question. I have not checked but I will.
> 
> FYI EPICS has set its limit to 64 because of this issue with select. I suspect
> they do not want to hack their select calls like this and I understand them
> taking that position.

Yes, I see the issue. The equate only works if the file ops count is the same as
the newlib defined fd_set.

Chris