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

[Chris Johns]

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.

Chris