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

[Christian MAUDERER]

On 2023-05-24 02:33, Chris Johns wrote:
> 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

Thanks for the confirmation. Do you see a better solution than the 
memcpy in my patch?

Best regards

Christian
-- 
--------------------------------------------
embedded brains GmbH & Co. KG
Herr Christian MAUDERER
Dornierstr. 4
82178 Puchheim
Germany
email:  christian.mauderer at embedded-brains.de
phone:  +49-89-18 94 741 - 18
mobile: +49-176-152 206 08

Registergericht: Amtsgericht München
Registernummer: HRA 117265
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/