[3/5] resolv: Fix file handle leak in __resolv_conf_load [BZ #25429]
diff mbox series

Message ID 4d8628bf1d8630c16de30d47732058ec82293429.1579631655.git.fweimer@redhat.com
State New
Headers show
Series
  • Race condition in /etc/resolv.conf reloading (bug 25420)
Related show

Commit Message

Florian Weimer Jan. 21, 2020, 6:41 p.m. UTC
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(-)

Comments

Adhemerval Zanella Feb. 13, 2020, 9 p.m. UTC | #1
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.
Florian Weimer Feb. 13, 2020, 9:08 p.m. UTC | #2
* 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?

Thanks,
Florian
Adhemerval Zanella Feb. 13, 2020, 9:30 p.m. UTC | #3
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>

Patch
diff mbox series

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;
 }