[PATCH] serialize access pci_get_strings function with a mutex

[wangjian (AN)]

Ok. I agree.

On 2021/3/26 17:10, Michal Privoznik wrote:
> There's no need to CC random developers - we are subscribed to the list.
> 
> On 3/26/21 4:21 AM, wangjian wrote:> serialize access pci_get_strings function with a mutex.> > nodedev-init thread:
>> nodeStateInitializeEnumerate ->
>>    udevEnumerateDevices->
>>      udevProcessDeviceListEntry ->
>>        udevAddOneDevice ->
>>          udevGetDeviceDetails->
>>            udevProcessPCI ->
>>              udevTranslatePCIIds ->
>>                pci_get_strings -> (libpciaccess)
>>                  find_device_name ->
>>                    populate_vendor ->
>>                      d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
>>                      vend->num_devices++;
>>
>> udev-event thread:
>> udevEventHandleThread ->
>>    udevHandleOneDevice ->
>>      udevAddOneDevice->
>>        udevGetDeviceDetails->
>>          udevProcessPCI ->
>>            udevTranslatePCIIds ->
>>              pci_get_strings -> (libpciaccess)
>>                find_device_name ->
>>                  populate_vendor ->
>>                    d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
>>                    vend->num_devices++;
>>
>> Since the functions provided by libpciaccess are not thread-safe,
>> when the udev-event and nodedev-init threads of libvirt call
>> the pci_get_strings function provided by libpaciaccess at the same time.
>> It will appear that for the same address, realloc a large space first,
>> and then realloc a small space, which triggers the 'invalid next size' of realloc,
>> leading to the thread core.
>>
>> Signed-off-by: WangJian <wangjian161 at huawei.com>
>> ---
>>   src/node_device/node_device_udev.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
>> index 3f28858..cc752ba 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -331,6 +331,7 @@ udevGenerateDeviceName(struct udev_device *device,
>>       return 0;
>>   }
>>
>> +static pthread_mutex_t g_pciaccess_mutex = PTHREAD_MUTEX_INITIALIZER;
> 
> We tend to use virMutex (even though it is pthread_mutex under the hood).
> 
>>
>>   static int
>>   udevTranslatePCIIds(unsigned int vendor,
>> @@ -350,11 +351,13 @@ udevTranslatePCIIds(unsigned int vendor,
>>       m.match_data = 0;
>>
>>       /* pci_get_strings returns void */
>> +    pthread_mutex_lock(&g_pciaccess_mutex);
>>       pci_get_strings(&m,
>>                       &device_name,
>>                       &vendor_name,
>>                       NULL,
>>                       NULL);
>> +    pthread_mutex_unlock(&g_pciaccess_mutex);
>>
>>       *vendor_string = g_strdup(vendor_name);
>>       *product_string = g_strdup(device_name);
>>
> 
> Apart from that, the patch is correct. I'd squash this in and push if you agree:
> 
> diff --git i/src/node_device/node_device_udev.c w/src/node_device/node_device_udev.c
> index cc752bad32..3daa5c90ad 100644
> --- i/src/node_device/node_device_udev.c
> +++ w/src/node_device/node_device_udev.c
> @@ -331,7 +331,7 @@ udevGenerateDeviceName(struct udev_device *device,
>      return 0;
>  }
> 
> -static pthread_mutex_t g_pciaccess_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static virMutex pciaccessMutex = VIR_MUTEX_INITIALIZER;
> 
>  static int
>  udevTranslatePCIIds(unsigned int vendor,
> @@ -350,14 +350,14 @@ udevTranslatePCIIds(unsigned int vendor,
>      m.device_class_mask = 0;
>      m.match_data = 0;
> 
> -    /* pci_get_strings returns void */
> -    pthread_mutex_lock(&g_pciaccess_mutex);
> +    /* pci_get_strings returns void and unfortunately is not thread safe. */
> +    virMutexLock(&pciaccessMutex);
>      pci_get_strings(&m,
>                      &device_name,
>                      &vendor_name,
>                      NULL,
>                      NULL);
> -    pthread_mutex_unlock(&g_pciaccess_mutex);
> +    virMutexUnlock(&pciaccessMutex);
> 
>      *vendor_string = g_strdup(vendor_name);
>      *product_string = g_strdup(device_name);
> 
> 
> Michal
> 
> .
>