GNU bug report logs - #32604
26.1.50; memory leak in connect_network_socket

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32604 in the body.
You can then email your comments to 32604 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.1.50; memory leak in connect_network_socket
Date: Sat, 1 Sep 2018 14:38:51 +0900

In connect_network_socket (in process.c), the memory pointed to
by the variable `sa’ doesn’t seem to be deallocated.

  3328	  struct sockaddr *sa = NULL;
	:
  3347	  while (!NILP (addrinfos))
  3348	    {
	:
  3359	      if (sa)
  3360		free (sa);
  3361	      sa = xmalloc (addrlen);
	:
  3533	    }
	:

The following patch would fix the leak:

diff --git a/src/process.c b/src/process.c
index 676f38446e..e5b70ca058 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3576,6 +3576,8 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
 #endif
     }
 
+  xfree (sa);
+
   if (s < 0)
     {
       const char *err = (p->is_server


Also, line 3359-3361 above could be:
      sa = xrealloc (addrlen);

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

In GNU Emacs 26.1.50 (build 1, x86_64-apple-darwin16.7.0)
 of 2018-09-01 built on YAMAMOTO-no-iMac-5K.local
Repository revision: ac7936cb8f4d4d6706535bfcea0d97741c2ca15f
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --without-ns --without-x-toolkit --without-gnutls
 --with-jpeg=no --with-gif=no --with-tiff=no
 PKG_CONFIG=/opt/local/bin/pkg-config
 PKG_CONFIG_PATH=/usr/lib/pkgconfig:/opt/X11/lib/pkgconfig:/usr/local/lib/pkgconfig
 PKG_CONFIG_LIBDIR= --enable-checking=yes,glyphs
 --enable-check-lisp-object-type CFLAGS=-g'

Configured features:
XPM PNG NOTIFY ACL LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB OLDXMENU
X11 XDBE XIM THREADS

Important settings:
  value of $LANG: ja_JP.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils elec-pair time-date
mule-util japan-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue
dynamic-setting font-render-setting x multi-tty make-network-process
emacs)

Memory information:
((conses 16 97309 7262)
 (symbols 48 20210 2)
 (miscs 40 64 126)
 (strings 32 28170 1559)
 (string-bytes 1 745551)
 (vectors 16 14728)
 (vector-slots 8 572806 12294)
 (floats 8 54 90)
 (intervals 56 261 13)
 (buffers 992 12))

Message #8 received at 32604 <at> debbugs.gnu.org (full text, mbox):

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: 32604 <at> debbugs.gnu.org
Subject: Re: bug#32604: 26.1.50; memory leak in connect_network_socket
Date: Sat, 01 Sep 2018 14:46:38 +0900

>   3328	  struct sockaddr *sa = NULL;

>   3359	      if (sa)
>   3360		free (sa);
>   3361	      sa = xmalloc (addrlen);

> Also, line 3359-3361 above could be:
>       sa = xrealloc (addrlen);

Oops, I meant

      sa = xrealloc (sa, addrlen);

of course.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #11 received at 32604 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Cc: 32604 <at> debbugs.gnu.org
Subject: Re: bug#32604: 26.1.50; memory leak in connect_network_socket
Date: Sun, 02 Sep 2018 13:55:46 -0400

YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp> writes:

> In connect_network_socket (in process.c), the memory pointed to
> by the variable `sa’ doesn’t seem to be deallocated.
>
>   3328	  struct sockaddr *sa = NULL;
> 	:
>   3347	  while (!NILP (addrinfos))
>   3348	    {
> 	:
>   3359	      if (sa)
>   3360		free (sa);
>   3361	      sa = xmalloc (addrlen);
> 	:
>   3533	    }
> 	:
>
> The following patch would fix the leak:

> +  xfree (sa);

I think we would need

    record_unwind_protect_ptr (xfree, sa);

to handle the case where an error is signaled.  Similar to how the
socket closing is handled:

      /* Make us close S if quit.  */
      record_unwind_protect_int (close_file_unwind, s);

Message #14 received at 32604 <at> debbugs.gnu.org (full text, mbox):

From: mituharu <at> math.s.chiba-u.ac.jp
To: "Noam Postavsky" <npostavs <at> gmail.com>
Cc: 32604 <at> debbugs.gnu.org
Subject: Re: bug#32604: 26.1.50; memory leak in connect_network_socket
Date: Mon, 3 Sep 2018 15:02:00 +0900

> YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp> writes:
>
>> In connect_network_socket (in process.c), the memory pointed to
>> by the variable `sa’ doesn’t seem to be deallocated.
>>
>>   3328  struct sockaddr *sa = NULL;
>> :
>>   3347  while (!NILP (addrinfos))
>>   3348    {
>> :
>>   3359      if (sa)
>>   3360free (sa);
>>   3361      sa = xmalloc (addrlen);
>> :
>>   3533    }
>> :
>>
>> The following patch would fix the leak:
>
>> +  xfree (sa);
>
> I think we would need
>
>     record_unwind_protect_ptr (xfree, sa);
>
> to handle the case where an error is signaled.  Similar to how the
> socket closing is handled:
>
>       /* Make us close S if quit.  */
>       record_unwind_protect_int (close_file_unwind, s);
>

Indeed.  Could someone double-check the patch below?

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

diff --git a/src/process.c b/src/process.c
index 676f38446e..ff53b86844 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3322,6 +3322,7 @@ connect_network_socket (Lisp_Object proc,
Lisp_Object addrinfos,
                         Lisp_Object use_external_socket_p)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
+  ptrdiff_t count1 UNINIT;
   int s = -1, outch, inch;
   int xerrno = 0;
   int family;
@@ -3344,6 +3345,9 @@ connect_network_socket (Lisp_Object proc,
Lisp_Object addrinfos,
   /* Do this in case we never enter the while-loop below.  */
   s = -1;

+  record_unwind_protect_nothing ();
+  count1 = SPECPDL_INDEX ();
+
   while (!NILP (addrinfos))
     {
       Lisp_Object addrinfo = XCAR (addrinfos);
@@ -3356,9 +3360,8 @@ connect_network_socket (Lisp_Object proc,
Lisp_Object addrinfos,
 #endif

       addrlen = get_lisp_to_sockaddr_size (ip_address, &family);
-      if (sa)
-	free (sa);
-      sa = xmalloc (addrlen);
+      sa = xrealloc (sa, addrlen);
+      set_unwind_protect_ptr (count, xfree, sa);
       conv_lisp_to_sockaddr (family, ip_address, sa, addrlen);

       s = socket_to_use;
@@ -3520,7 +3523,7 @@ connect_network_socket (Lisp_Object proc,
Lisp_Object addrinfos,
 #endif /* !WINDOWSNT */

       /* Discard the unwind protect closing S.  */
-      specpdl_ptr = specpdl + count;
+      specpdl_ptr = specpdl + count1;
       emacs_close (s);
       s = -1;
       if (0 <= socket_to_use)
@@ -3591,6 +3594,7 @@ connect_network_socket (Lisp_Object proc,
Lisp_Object addrinfos,
 	  Lisp_Object data = get_file_errno_data (err, contact, xerrno);

 	  pset_status (p, list2 (Fcar (data), Fcdr (data)));
+	  unbind_to (count, Qnil);
 	  return;
 	}

@@ -3610,7 +3614,7 @@ connect_network_socket (Lisp_Object proc,
Lisp_Object addrinfos,
   p->outfd = outch;

   /* Discard the unwind protect for closing S, if any.  */
-  specpdl_ptr = specpdl + count;
+  specpdl_ptr = specpdl + count1;

   if (p->is_server && p->socktype != SOCK_DGRAM)
     pset_status (p, Qlisten);
@@ -3671,6 +3675,7 @@ connect_network_socket (Lisp_Object proc,
Lisp_Object addrinfos,
     }
 #endif

+  unbind_to (count, Qnil);
 }

 /* Create a network stream/datagram client/server process.  Treated

Message #17 received at 32604 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: mituharu <at> math.s.chiba-u.ac.jp
Cc: 32604 <at> debbugs.gnu.org
Subject: Re: bug#32604: 26.1.50; memory leak in connect_network_socket
Date: Tue, 04 Sep 2018 19:19:48 -0400

Looks good to me; a couple of minor suggestions below.

mituharu <at> math.s.chiba-u.ac.jp writes:

> @@ -3322,6 +3322,7 @@ connect_network_socket (Lisp_Object proc,
> Lisp_Object addrinfos,
>                          Lisp_Object use_external_socket_p)
>  {
>    ptrdiff_t count = SPECPDL_INDEX ();
> +  ptrdiff_t count1 UNINIT;
>    int s = -1, outch, inch;
>    int xerrno = 0;
>    int family;
> @@ -3344,6 +3345,9 @@ connect_network_socket (Lisp_Object proc,
> Lisp_Object addrinfos,
>    /* Do this in case we never enter the while-loop below.  */
>    s = -1;
>
> +  record_unwind_protect_nothing ();
> +  count1 = SPECPDL_INDEX ();

Since we assume a C99 compiler now, you could just do

    ptrdiff_t count1 = SPECPDL_INDEX ();

without having the UNINIT thing.  Also, since free is harmless on a NULL
pointer, you could just record an unwind protect at the top once,
without having the nothing state, I think.

Message #20 received at 32604 <at> debbugs.gnu.org (full text, mbox):

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 32604 <at> debbugs.gnu.org
Subject: Re: bug#32604: 26.1.50; memory leak in connect_network_socket
Date: Thu, 06 Sep 2018 08:50:33 +0900

On Wed, 05 Sep 2018 08:19:48 +0900,
Noam Postavsky wrote:

> 
> Looks good to me; a couple of minor suggestions below.
> 
> mituharu <at> math.s.chiba-u.ac.jp writes:
> 
> > @@ -3322,6 +3322,7 @@ connect_network_socket (Lisp_Object proc,
> > Lisp_Object addrinfos,
> >                          Lisp_Object use_external_socket_p)
> >  {
> >    ptrdiff_t count = SPECPDL_INDEX ();
> > +  ptrdiff_t count1 UNINIT;
> >    int s = -1, outch, inch;
> >    int xerrno = 0;
> >    int family;
> > @@ -3344,6 +3345,9 @@ connect_network_socket (Lisp_Object proc,
> > Lisp_Object addrinfos,
> >    /* Do this in case we never enter the while-loop below.  */
> >    s = -1;
> >
> > +  record_unwind_protect_nothing ();
> > +  count1 = SPECPDL_INDEX ();
> 
> Since we assume a C99 compiler now, you could just do
> 
>     ptrdiff_t count1 = SPECPDL_INDEX ();
> 
> without having the UNINIT thing.  Also, since free is harmless on a NULL
> pointer, you could just record an unwind protect at the top once,
> without having the nothing state, I think.

Thanks for the comments.  With C99, I would probably gather related
things like:

  struct sockaddr *sa = NULL;
  ptrdiff_t count = SPECPDL_INDEX ();
  record_unwind_protect_nothing ();
	:
  while (!NILP (addrinfos))
    {
	:
      sa = xrealloc (sa, addrlen);
      set_unwind_protect_ptr (count, xfree, sa);
	:
    }
	:
  unbind_to (count, Qnil);

This looks much more idiomatic.  We need to update specbinding
according to (potential) change of the value of `sa' by xrealloc call.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp
   
diff --git a/src/process.c b/src/process.c
index 676f38446e..b0a327229c 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3321,11 +3321,9 @@ static void
 connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
                         Lisp_Object use_external_socket_p)
 {
-  ptrdiff_t count = SPECPDL_INDEX ();
   int s = -1, outch, inch;
   int xerrno = 0;
   int family;
-  struct sockaddr *sa = NULL;
   int ret;
   ptrdiff_t addrlen;
   struct Lisp_Process *p = XPROCESS (proc);
@@ -3344,6 +3342,11 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
   /* Do this in case we never enter the while-loop below.  */
   s = -1;
 
+  struct sockaddr *sa = NULL;
+  ptrdiff_t count = SPECPDL_INDEX ();
+  record_unwind_protect_nothing ();
+  ptrdiff_t count1 = SPECPDL_INDEX ();
+
   while (!NILP (addrinfos))
     {
       Lisp_Object addrinfo = XCAR (addrinfos);
@@ -3356,9 +3359,8 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
 #endif
 
       addrlen = get_lisp_to_sockaddr_size (ip_address, &family);
-      if (sa)
-	free (sa);
-      sa = xmalloc (addrlen);
+      sa = xrealloc (sa, addrlen);
+      set_unwind_protect_ptr (count, xfree, sa);
       conv_lisp_to_sockaddr (family, ip_address, sa, addrlen);
 
       s = socket_to_use;
@@ -3520,7 +3522,7 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
 #endif /* !WINDOWSNT */
 
       /* Discard the unwind protect closing S.  */
-      specpdl_ptr = specpdl + count;
+      specpdl_ptr = specpdl + count1;
       emacs_close (s);
       s = -1;
       if (0 <= socket_to_use)
@@ -3591,6 +3593,7 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
 	  Lisp_Object data = get_file_errno_data (err, contact, xerrno);
 
 	  pset_status (p, list2 (Fcar (data), Fcdr (data)));
+	  unbind_to (count, Qnil);
 	  return;
 	}
 
@@ -3610,7 +3613,7 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
   p->outfd = outch;
 
   /* Discard the unwind protect for closing S, if any.  */
-  specpdl_ptr = specpdl + count;
+  specpdl_ptr = specpdl + count1;
 
   if (p->is_server && p->socktype != SOCK_DGRAM)
     pset_status (p, Qlisten);
@@ -3671,6 +3674,7 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
     }
 #endif
 
+  unbind_to (count, Qnil);
 }
 
 /* Create a network stream/datagram client/server process.  Treated

Message #23 received at 32604 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Cc: 32604 <at> debbugs.gnu.org
Subject: Re: bug#32604: 26.1.50; memory leak in connect_network_socket
Date: Wed, 05 Sep 2018 20:04:27 -0400

YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp> writes:

> We need to update specbinding according to (potential) change of the
> value of `sa' by xrealloc call.

Yes, you're right.  I was confused and somehow dreamed up an extra level
of indirection in the unwind_protect mechanism.

Message #28 received at 32604-done <at> debbugs.gnu.org (full text, mbox):

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: 32604-done <at> debbugs.gnu.org
Subject: Re: bug#32604: 26.1.50; memory leak in connect_network_socket
Date: Fri, 07 Sep 2018 08:41:40 +0900

On Thu, 06 Sep 2018 09:04:27 +0900,
Noam Postavsky wrote:

> YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp> writes:
> 
> > We need to update specbinding according to (potential) change of the
> > value of `sa' by xrealloc call.
> 
> Yes, you're right.  I was confused and somehow dreamed up an extra level
> of indirection in the unwind_protect mechanism.

Installed to the emacs-26 branch.  Closing.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.