Re: [PATCH] libxl: Check for regcomp failure

[Jim Fehlig <jfehlig@xxxxxxxx>]

Jim Fehlig wrote:
> Michal Privoznik wrote:
>   
>> On 04.09.2013 10:29, Michal Privoznik wrote:
>>   
>>     
>>> On 04.09.2013 08:37, Jim Fehlig wrote:
>>>     
>>>       
>>>> Change libxlGetAutoballoonConf() function to return an int
>>>> for success/failure, and fail if regcomp fails.
>>>>
>>>> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
>>>> ---
>>>>  src/libxl/libxl_conf.c | 30 +++++++++++++++++++-----------
>>>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>>> index fcb278b..a634476 100644
>>>> --- a/src/libxl/libxl_conf.c
>>>> +++ b/src/libxl/libxl_conf.c
>>>> @@ -1014,21 +1014,28 @@ error:
>>>>      return -1;
>>>>  }
>>>>  
>>>> -static bool
>>>> -libxlGetAutoballoonConf(libxlDriverConfigPtr cfg)
>>>> +static int
>>>> +libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, bool *autoballoon)
>>>>  {
>>>>      regex_t regex;
>>>> -    int ret;
>>>> +    int res;
>>>> +
>>>> +    if ((res = regcomp(&regex,
>>>> +                      "(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )",
>>>> +                       REG_NOSUB | REG_EXTENDED)) != 0) {
>>>> +        char error[100];
>>>> +        regerror(res, &regex, error, sizeof(error));
>>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                       _("Failed to compile regex %s"),
>>>> +                       error);
>>>>       
>>>>         
>>> My only concern is, that 100 bytes may not be enough sometimes. But on
>>> the other hand, there's another occurrence of regerror and there's
>>> buffer of the exact size passed too. So I'm comfortable enough to give
>>> you my ACK.
>>>
>>>     
>>>       
>> Hit send prematurely, the other occurrence is doing regfree(&regex);
>> even on error path (in fact, just on the error path). I must confess I
>> don't get it (in case of error, there should not be any regex created,
>> should it?). So - do we need to regfree() even on error?
>>   
>>     
>
> Yeah, good question.  I found a few occurrences of regcomp() and friends
> throughout the sources and most seem to do regfree() even when regcomp()
> fails.  The man page is not very clear, but the notes on regfree()
> suggest it is not necessary
>
>   POSIX Pattern Buffer Freeing
>     Supplying regfree() with a precompiled pattern buffer, preg will
>     free the memory allocated to the pattern buffer by the compiling
>     process, regcomp().
>
> But does the pattern buffer contain any allocated memory when regcomp()
> fails?  The notes on regcomp() are not clear about this.
>   

The System Interfaces volume of POSIX.1-2008 [1] says this about
regcomp() return value

Upon successful completion, the regcomp() function shall return 0.
Otherwise, it shall return an integer value indicating an error as
described in /<regex.h>/
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>,
and the content of preg is undefined. If a code is returned, the
interpretation shall be as given in /<regex.h>/
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>.

I don't think we want to call regfree() on an undefined preg right?

Regards,
Jim

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list