Re: [PATCH 3/5] resolv: Fix file handle leak in __resolv_conf_load [BZ #25429]

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 13/02/2020 18:08, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 21/01/2020 15:41, Florian Weimer wrote:
>>> res_vinit_1 did not close the stream on errors, only on success.
>>> This change moves closing the stream to __resolv_conf_load, for both
>>> the success and error cases.
>>>
>>> Fixes commit 89f187a40fc0ad4e22838526bfe34d73f758b776 ("resolv: Use
>>> getline for configuration file reading in res_vinit_1") and commit
>>> 3f853f22c87f0b671c0366eb290919719fa56c0e ("resolv: Lift domain search
>>> list limits [BZ #19569] [BZ #21475]"), where memory allocation was
>>> introduced into res_vinit_1.
>>> ---
>>>  resolv/res_init.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/resolv/res_init.c b/resolv/res_init.c
>>> index 95dce098aa..09345718cd 100644
>>> --- a/resolv/res_init.c
>>> +++ b/resolv/res_init.c
>>> @@ -508,7 +508,6 @@ res_vinit_1 (FILE *fp, struct resolv_conf_parser *parser)
>>>                continue;
>>>              }
>>>          }
>>> -      fclose (fp);
>>>      }
>>>    if (__glibc_unlikely (nameserver_list_size (&parser->nameserver_list) == 0))
>>>      {
>>> @@ -593,6 +592,13 @@ __resolv_conf_load (struct __res_state *preinit)
>>>      }
>>>    resolv_conf_parser_free (&parser);
>>>  
>>> +  if (fp != NULL)
>>> +    {
>>> +      int saved_errno = errno;
>>> +      fclose (fp);
>>> +      __set_errno (saved_errno);
>>> +    }
>>> +
>>>    return conf;
>>>  }
>>
>> Why not close the FILE on __resolv_conf_load? It make the FILE object 
>> cleanup as close as its creation, which usually improves readability.
> 
> Sorry, I don't understand.  Isn't this what the patch does?

Nevermind, for some reason I though it was doing on res_vinit_1.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>