[RFC] android/hal-audio: Fix leaving open socket

[RFC] android/hal-audio: Fix leaving open socket

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

When getting out of the poll loop we shall close socket always.
---
 android/hal-audio.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index bfc102f..439b583 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -1346,14 +1346,12 @@ static void *ipc_handler(void *data)
 		/* Check if socket is still alive. Empty while loop.*/
 		while (poll(&pfd, 1, -1) < 0 && errno == EINTR);
 
-		if (pfd.revents & (POLLHUP | POLLERR | POLLNVAL)) {
-			info("Audio HAL: Socket closed");
+		info("Audio HAL: Socket closed");
 
-			pthread_mutex_lock(&sk_mutex);
-			close(audio_sk);
-			audio_sk = -1;
-			pthread_mutex_unlock(&sk_mutex);
-		}
+		pthread_mutex_lock(&sk_mutex);
+		close(audio_sk);
+		audio_sk = -1;
+		pthread_mutex_unlock(&sk_mutex);
 	}
 
 	/* audio_sk is closed at this point, just cleanup endpoints states */
-- 
1.8.3.2

Re: [RFC] android/hal-audio: Fix leaving open socket

[Luiz Augusto von Dentz]

Hi Andrei,

On Thu, Jun 5, 2014 at 3:31 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> When getting out of the poll loop we shall close socket always.
> ---
>  android/hal-audio.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/android/hal-audio.c b/android/hal-audio.c
> index bfc102f..439b583 100644
> --- a/android/hal-audio.c
> +++ b/android/hal-audio.c
> @@ -1346,14 +1346,12 @@ static void *ipc_handler(void *data)
>                 /* Check if socket is still alive. Empty while loop.*/
>                 while (poll(&pfd, 1, -1) < 0 && errno == EINTR);
>
> -               if (pfd.revents & (POLLHUP | POLLERR | POLLNVAL)) {
> -                       info("Audio HAL: Socket closed");
> +               info("Audio HAL: Socket closed");
>
> -                       pthread_mutex_lock(&sk_mutex);
> -                       close(audio_sk);
> -                       audio_sk = -1;
> -                       pthread_mutex_unlock(&sk_mutex);
> -               }
> +               pthread_mutex_lock(&sk_mutex);
> +               close(audio_sk);
> +               audio_sk = -1;
> +               pthread_mutex_unlock(&sk_mutex);
>         }
>
>         /* audio_sk is closed at this point, just cleanup endpoints states */
> --
> 1.8.3.2

Im not quite sure what this does fix, I mean I do understand that the
if statement might not be required after all we have since we have
pfd.events = POLLHUP | POLLERR | POLLNVAL but removing it is not a fix
more a cleanup.


-- 
Luiz Augusto von Dentz

Re: [RFC] android/hal-audio: Fix leaving open socket

[Andrei Emeltchenko]

Hi Luiz,

On Mon, Jun 09, 2014 at 04:31:13PM +0300, Luiz Augusto von Dentz wrote:
> Hi Andrei,
> 
> On Thu, Jun 5, 2014 at 3:31 PM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > When getting out of the poll loop we shall close socket always.
> > ---
> >  android/hal-audio.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/android/hal-audio.c b/android/hal-audio.c
> > index bfc102f..439b583 100644
> > --- a/android/hal-audio.c
> > +++ b/android/hal-audio.c
> > @@ -1346,14 +1346,12 @@ static void *ipc_handler(void *data)
> >                 /* Check if socket is still alive. Empty while loop.*/
> >                 while (poll(&pfd, 1, -1) < 0 && errno == EINTR);
> >
> > -               if (pfd.revents & (POLLHUP | POLLERR | POLLNVAL)) {
> > -                       info("Audio HAL: Socket closed");
> > +               info("Audio HAL: Socket closed");
> >
> > -                       pthread_mutex_lock(&sk_mutex);
> > -                       close(audio_sk);
> > -                       audio_sk = -1;
> > -                       pthread_mutex_unlock(&sk_mutex);
> > -               }
> > +               pthread_mutex_lock(&sk_mutex);
> > +               close(audio_sk);
> > +               audio_sk = -1;
> > +               pthread_mutex_unlock(&sk_mutex);
> >         }
> >
> >         /* audio_sk is closed at this point, just cleanup endpoints states */
> > --
> > 1.8.3.2
> 
> Im not quite sure what this does fix, I mean I do understand that the
> if statement might not be required after all we have since we have
> pfd.events = POLLHUP | POLLERR | POLLNVAL but removing it is not a fix
> more a cleanup.

Yes, it might be more cleanup. But the fact you check for EINTR implies
that there are other errors unchecked in which case we have resource leak.

Best regards 
Andrei Emeltchenko