Re: [patch] Fix resource leak in netcat

2018-10-02 Thread Theo Buehler 
On Tue, Oct 02, 2018 at 02:38:03AM +0200, Alexander Bluhm wrote:
> On Sun, Sep 30, 2018 at 11:55:58AM +0800, Nan Xiao wrote:
> > The following patch fixed the resource leak when netcat works as a TLS
> > server. Sorry if I am wrong, thanks!
> 
> There is another tls leak on the client side after
> if (s == -1)
> continue;
> 
> To fix both I would do the tls_free() after close() consistently.

ok

Re: [patch] Fix resource leak in netcat

2018-10-02 Thread Nan Xiao 
Hi bluhm,

Thanks very much for your reply!

Yes, your patch covers all the corner cases, thanks!

On 10/2/2018 8:38 AM, Alexander Bluhm wrote:
> On Sun, Sep 30, 2018 at 11:55:58AM +0800, Nan Xiao wrote:
>> The following patch fixed the resource leak when netcat works as a TLS
>> server. Sorry if I am wrong, thanks!
> 
> There is another tls leak on the client side after
> if (s == -1)
> continue;
> 
> To fix both I would do the tls_free() after close() consistently.
> 
> bluhm
> 
> Index: usr.bin/nc/netcat.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.194
> diff -u -p -r1.194 netcat.c
> --- usr.bin/nc/netcat.c   7 Sep 2018 09:55:29 -   1.194
> +++ usr.bin/nc/netcat.c   2 Oct 2018 00:35:03 -
> @@ -543,8 +543,6 @@ main(int argc, char *argv[])
>   err(1, "pledge");
>   }
>   if (lflag) {
> - struct tls *tls_cctx = NULL;
> - int connfd;
>   ret = 0;
>  
>   if (family == AF_UNIX) {
> @@ -603,6 +601,9 @@ main(int argc, char *argv[])
>  
>   readwrite(s, NULL);
>   } else {
> + struct tls *tls_cctx = NULL;
> + int connfd;
> +
>   len = sizeof(cliaddr);
>   connfd = accept4(s, (struct sockaddr *),
>   , SOCK_NONBLOCK);
> @@ -618,12 +619,10 @@ main(int argc, char *argv[])
>   readwrite(connfd, tls_cctx);
>   if (!usetls)
>   readwrite(connfd, NULL);
> - if (tls_cctx) {
> + if (tls_cctx)
>   timeout_tls(s, tls_cctx, tls_close);
> - tls_free(tls_cctx);
> - tls_cctx = NULL;
> - }
>   close(connfd);
> + tls_free(tls_cctx);
>   }
>   if (family == AF_UNIX && uflag) {
>   if (connect(s, NULL, 0) < 0)
> @@ -657,6 +656,8 @@ main(int argc, char *argv[])
>   for (s = -1, i = 0; portlist[i] != NULL; i++) {
>   if (s != -1)
>   close(s);
> + tls_free(tls_ctx);
> + tls_ctx = NULL;
>  
>   if (usetls) {
>   if ((tls_ctx = tls_client()) == NULL)
> @@ -707,18 +708,15 @@ main(int argc, char *argv[])
>   tls_setup_client(tls_ctx, s, host);
>   if (!zflag)
>   readwrite(s, tls_ctx);
> - if (tls_ctx) {
> + if (tls_ctx)
>   timeout_tls(s, tls_ctx, tls_close);
> - tls_free(tls_ctx);
> - tls_ctx = NULL;
> - }
>   }
>   }
>   }
>  
>   if (s != -1)
>   close(s);
> -
> + tls_free(tls_ctx);
>   tls_config_free(tls_cfg);
>  
>   return ret;
> 

-- 
Best Regards
Nan Xiao(肖楠)

Re: [patch] Fix resource leak in netcat

2018-10-01 Thread Alexander Bluhm 
On Sun, Sep 30, 2018 at 11:55:58AM +0800, Nan Xiao wrote:
> The following patch fixed the resource leak when netcat works as a TLS
> server. Sorry if I am wrong, thanks!

There is another tls leak on the client side after
if (s == -1)
continue;

To fix both I would do the tls_free() after close() consistently.

bluhm

Index: usr.bin/nc/netcat.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.194
diff -u -p -r1.194 netcat.c
--- usr.bin/nc/netcat.c 7 Sep 2018 09:55:29 -   1.194
+++ usr.bin/nc/netcat.c 2 Oct 2018 00:35:03 -
@@ -543,8 +543,6 @@ main(int argc, char *argv[])
err(1, "pledge");
}
if (lflag) {
-   struct tls *tls_cctx = NULL;
-   int connfd;
ret = 0;
 
if (family == AF_UNIX) {
@@ -603,6 +601,9 @@ main(int argc, char *argv[])
 
readwrite(s, NULL);
} else {
+   struct tls *tls_cctx = NULL;
+   int connfd;
+
len = sizeof(cliaddr);
connfd = accept4(s, (struct sockaddr *),
, SOCK_NONBLOCK);
@@ -618,12 +619,10 @@ main(int argc, char *argv[])
readwrite(connfd, tls_cctx);
if (!usetls)
readwrite(connfd, NULL);
-   if (tls_cctx) {
+   if (tls_cctx)
timeout_tls(s, tls_cctx, tls_close);
-   tls_free(tls_cctx);
-   tls_cctx = NULL;
-   }
close(connfd);
+   tls_free(tls_cctx);
}
if (family == AF_UNIX && uflag) {
if (connect(s, NULL, 0) < 0)
@@ -657,6 +656,8 @@ main(int argc, char *argv[])
for (s = -1, i = 0; portlist[i] != NULL; i++) {
if (s != -1)
close(s);
+   tls_free(tls_ctx);
+   tls_ctx = NULL;
 
if (usetls) {
if ((tls_ctx = tls_client()) == NULL)
@@ -707,18 +708,15 @@ main(int argc, char *argv[])
tls_setup_client(tls_ctx, s, host);
if (!zflag)
readwrite(s, tls_ctx);
-   if (tls_ctx) {
+   if (tls_ctx)
timeout_tls(s, tls_ctx, tls_close);
-   tls_free(tls_ctx);
-   tls_ctx = NULL;
-   }
}
}
}
 
if (s != -1)
close(s);
-
+   tls_free(tls_ctx);
tls_config_free(tls_cfg);
 
return ret;

Re: [patch] Fix resource leak in netcat

2018-10-01 Thread Nan Xiao 
Ping tech@,

Any developer can comment on this patch? I think it is indeed a bug.

Thanks!

On 9/30/2018 11:55 AM, Nan Xiao wrote:
> Hi tech@,
> 
> The following patch fixed the resource leak when netcat works as a TLS
> server. Sorry if I am wrong, thanks!
> 
> 
> Index: netcat.c
> ===
> RCS file: /cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.194
> diff -u -p -r1.194 netcat.c
> --- netcat.c  7 Sep 2018 09:55:29 -   1.194
> +++ netcat.c  30 Sep 2018 03:50:03 -
> @@ -633,6 +633,11 @@ main(int argc, char *argv[])
>   if (!kflag)
>   break;
>   }
> +
> + if (tls_ctx) {
> + tls_free(tls_ctx);
> + tls_ctx = NULL;
> + }
>   } else if (family == AF_UNIX) {
>   ret = 0;
> 

-- 
Best Regards
Nan Xiao