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

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.
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(-)

-- 
2.24.1

Comments

Adhemerval Zanella Feb. 13, 2020, 9 p.m. | #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. | #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. | #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 --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;
 }