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

[Christian MAUDERER]

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?

Best regards

Christian

> 
> Chris

-- 
--------------------------------------------
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/