Re: [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases

["Harris, James R" <james.r.harris@intel.com>]

> On Aug 20, 2019, at 6:13 AM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
> 
>> On 16-Aug-19 1:13 PM, Jim Harris wrote:
>> The code checks both rte_mp_request_sync() return
>> code and that the number of messages in the reply
>> equals 1.  If rte_mp_request_sync() succeeds but
>> there was more than one message, those messages
>> would get leaked.
>> Found via code review by Anatoly Burakov of patches
>> that used the vhost code as a template for using
>> rte_mp_request_sync().
>> Signed-off-by: Jim Harris <james.r.harris@intel.com>
>> ---
>>  lib/librte_eal/linux/eal/eal_vfio.c |   16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>> diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
>> index 501c74f23..d9541b122 100644
>> --- a/lib/librte_eal/linux/eal/eal_vfio.c
>> +++ b/lib/librte_eal/linux/eal/eal_vfio.c
>> @@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
>>      int vfio_group_fd;
>>      char filename[PATH_MAX];
>>      struct rte_mp_msg mp_req, *mp_rep;
>> -    struct rte_mp_reply mp_reply;
>> +    struct rte_mp_reply mp_reply = {0};
>>      struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
>>      struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
>>  @@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
>>              RTE_LOG(ERR, EAL, "  bad VFIO group fd\n");
>>              vfio_group_fd = 0;
>>          }
>> -        free(mp_reply.msgs);
>>      }
>>  +    free(mp_reply.msgs);
> 
> That's not quite correct. This fixes the problem of missing free() when nb_received mismatches, but this /adds/ a problem of doing an unnecessary free() when rte_mp_request_sync() returns -1. Same for other places, i believe.

This would just resolve to free(NULL) in the -1 case.

Jim

> 
> -- 
> Thanks,
> Anatoly