[PATCH] slc: Fix file descriptor leak

[PATCH] slc: Fix file descriptor leak

[Ammar Faizi]

Arthur Lapz reported that he is seeing a blocking socket from the
public client side after the app side has closed the connection.

This turned out that we haven't closed the SLC socket file descriptor:

```
     Isolated                              Server
     --------                              ------

      fd_app --                         -- fd_public_client
               |----[ SLC circuit ]----|
  fd_circuit --                         -- fd_circuit

```
After the app in the isolated client has closed its fd, we didn't close
the fd_app and fd_circuit. That's why the server is still sleeping,
waiting for any response from the isolated area.

This was just a typo in if statements that caused the close() paths were
not taken. Simply fix the typo. Change "==" to "!=".

Link: https://t.me/lostcontrol_s1/16737
Link: https://t.me/lostcontrol_s1/16739 # Reproducer
Fixes: 04a86adffa415e8365d27929bbb826028e00bfd3 ("Initial commit")
Reported-by: Arthur Lapz <rlapz@gnuweeb.org>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 slc.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/slc.cpp b/slc.cpp
index bd836cb..5fcdd93 100644
--- a/slc.cpp
+++ b/slc.cpp
@@ -600,9 +600,9 @@ out_free:
 
 	socket_bridge(fd_pa, fd_pb);
 out:
-	if (fd_pa == -1)
+	if (fd_pa != -1)
 		close(fd_pa);
-	if (fd_pb == -1)
+	if (fd_pb != -1)
 		close(fd_pb);
 	return NULL;
 }
-- 
Ammar Faizi

Re: [PATCH] slc: Fix file descriptor leak

[Ammar Faizi]

On 6/6/22 9:27 AM, Ammar Faizi wrote:
> Arthur Lapz reported that he is seeing a blocking socket from the
> public client side after the app side has closed the connection.
> 
> This turned out that we haven't closed the SLC socket file descriptor:
> 
> ```
>       Isolated                              Server
>       --------                              ------
> 
>        fd_app --                         -- fd_public_client
>                 |----[ SLC circuit ]----|
>    fd_circuit --                         -- fd_circuit
> 
> ```
> After the app in the isolated client has closed its fd, we didn't close
> the fd_app and fd_circuit. That's why the server is still sleeping,
> waiting for any response from the isolated area.
> 
> This was just a typo in if statements that caused the close() paths were
> not taken. Simply fix the typo. Change "==" to "!=".
> 
> Link: https://t.me/lostcontrol_s1/16737
> Link: https://t.me/lostcontrol_s1/16739 # Reproducer
> Fixes: 04a86adffa415e8365d27929bbb826028e00bfd3 ("Initial commit")
> Reported-by: Arthur Lapz <rlapz@gnuweeb.org>
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---

@rlapz, can you test this patch and see if it's a cure?

-- 
Ammar Faizi

Re: [PATCH] slc: Fix file descriptor leak

[Arthur Lapz]

On Mon, 2022-06-06 at 09:28 +0700, Ammar Faizi wrote:
> On 6/6/22 9:27 AM, Ammar Faizi wrote:
> > Arthur Lapz reported that he is seeing a blocking socket from the
> > public client side after the app side has closed the connection.
> > 
> > This turned out that we haven't closed the SLC socket file
> > descriptor:
> > 
> > ```
> >       Isolated                              Server
> >       --------                              ------
> > 
> >        fd_app --                         -- fd_public_client
> >                 |----[ SLC circuit ]----|
> >    fd_circuit --                         -- fd_circuit
> > 
> > ```
> > After the app in the isolated client has closed its fd, we didn't
> > close
> > the fd_app and fd_circuit. That's why the server is still sleeping,
> > waiting for any response from the isolated area.
> > 
> > This was just a typo in if statements that caused the close() paths
> > were
> > not taken. Simply fix the typo. Change "==" to "!=".
> > 
> > Link: https://t.me/lostcontrol_s1/16737
> > Link: https://t.me/lostcontrol_s1/16739 # Reproducer
> > Fixes: 04a86adffa415e8365d27929bbb826028e00bfd3 ("Initial commit")
> > Reported-by: Arthur Lapz <rlapz@gnuweeb.org>
> > Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> > ---
> 
> @rlapz, can you test this patch and see if it's a cure?
> 

Unfortunately, this patch isn't a cure (still the same). :(
-- 
Arthur Lapz

Re: [PATCH] slc: Fix file descriptor leak

[Ammar Faizi]

On 6/6/22 9:42 AM, Arthur Lapz wrote:
> Unfortunately, this patch isn't a cure (still the same). :(

Not sure what is going on here, but the file descriptor bug
did really exist and is fixed by that patch.

The current SLC server looks like this:

   The "lost client" has been connected!
   fd_in is down
   send: Connection reset by peer
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe
   send: Broken pipe

I will investigate it later after doing some work.

-- 
Ammar Faizi

Re: [PATCH] slc: Fix file descriptor leak

[Ammar Faizi]

On 6/6/22 9:46 AM, Ammar Faizi wrote:
> On 6/6/22 9:42 AM, Arthur Lapz wrote:
>> Unfortunately, this patch isn't a cure (still the same). :(
[...]
> 
> I will investigate it later after doing some work.
> 

I think this is now fixed. I can access:

    https://test-rlapz.gnuweeb.org/

perfectly fine. How did you test this?

-- 
Ammar Faizi

Re: [PATCH] slc: Fix file descriptor leak

[Alviro Iskandar Setiawan]

Lmao, that's a silly bug. The fix looks good to me.

Reviewed-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>

tq

-- Viro

Re: [PATCH] slc: Fix file descriptor leak

[Arthur Lapz]

On Mon, 2022-06-06 at 09:51 +0700, Ammar Faizi wrote:
> On 6/6/22 9:46 AM, Ammar Faizi wrote:
> > On 6/6/22 9:42 AM, Arthur Lapz wrote:
> > > Unfortunately, this patch isn't a cure (still the same). :(
> [...]
> > 
> > I will investigate it later after doing some work.
> > 
> 
> I think this is now fixed. I can access:
> 
>     https://test-rlapz.gnuweeb.org/
> 
> perfectly fine. How did you test this?
> 
Yes it is.

> How did you test this?
I did nothing.

I guess it worked because you have restarted the `slc server`
-- 
Arthur Lapz

Re: [PATCH] slc: Fix file descriptor leak

[Ammar Faizi]

On 6/6/22 9:56 AM, Arthur Lapz wrote:
>> How did you test this?
> I did nothing.
> 
> I guess it worked because you have restarted the `slc server`

Yes, I have just restarted the SLC server. Do you have another
issue now?

-- 
Ammar Faizi

Re: [PATCH] slc: Fix file descriptor leak

[Arthur Lapz]

On Mon, 2022-06-06 at 09:57 +0700, Ammar Faizi wrote:
> On 6/6/22 9:56 AM, Arthur Lapz wrote:
> > > How did you test this?
> > I did nothing.
> > 
> > I guess it worked because you have restarted the `slc server`
> 
> Yes, I have just restarted the SLC server. Do you have another
> issue now?
> 
I don't have.
Thanks :3
-- 
Arthur Lapz

Re: [PATCH] slc: Fix file descriptor leak

[Ammar Faizi]

On 6/6/22 9:54 AM, Alviro Iskandar Setiawan wrote:
> Reviewed-by: Alviro Iskandar Setiawan<alviro.iskandar@gnuweeb.org>

Pushed out with your Reviewed-by tag.

   https://github.com/ammarfaizi2/slc/commit/d406993c3fcbfb7f6a24264c911731505d310163

Thanks to Arthur for reporting.
Thanks to Alviro for reviewing.

-- 
Ammar Faizi