* bug#32604: 26.1.50; memory leak in connect_network_socket @ 2018-09-01 5:38 YAMAMOTO Mitsuharu 2018-09-01 5:46 ` YAMAMOTO Mitsuharu 2018-09-02 17:55 ` Noam Postavsky 0 siblings, 2 replies; 8+ messages in thread From: YAMAMOTO Mitsuharu @ 2018-09-01 5:38 UTC (permalink / raw) To: 32604 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@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)) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#32604: 26.1.50; memory leak in connect_network_socket 2018-09-01 5:38 bug#32604: 26.1.50; memory leak in connect_network_socket YAMAMOTO Mitsuharu @ 2018-09-01 5:46 ` YAMAMOTO Mitsuharu 2018-09-02 17:55 ` Noam Postavsky 1 sibling, 0 replies; 8+ messages in thread From: YAMAMOTO Mitsuharu @ 2018-09-01 5:46 UTC (permalink / raw) To: 32604 > 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@math.s.chiba-u.ac.jp ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#32604: 26.1.50; memory leak in connect_network_socket 2018-09-01 5:38 bug#32604: 26.1.50; memory leak in connect_network_socket YAMAMOTO Mitsuharu 2018-09-01 5:46 ` YAMAMOTO Mitsuharu @ 2018-09-02 17:55 ` Noam Postavsky 2018-09-03 6:02 ` mituharu 1 sibling, 1 reply; 8+ messages in thread From: Noam Postavsky @ 2018-09-02 17:55 UTC (permalink / raw) To: YAMAMOTO Mitsuharu; +Cc: 32604 YAMAMOTO Mitsuharu <mituharu@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); ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#32604: 26.1.50; memory leak in connect_network_socket 2018-09-02 17:55 ` Noam Postavsky @ 2018-09-03 6:02 ` mituharu 2018-09-04 23:19 ` Noam Postavsky 0 siblings, 1 reply; 8+ messages in thread From: mituharu @ 2018-09-03 6:02 UTC (permalink / raw) To: Noam Postavsky; +Cc: 32604 > YAMAMOTO Mitsuharu <mituharu@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@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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#32604: 26.1.50; memory leak in connect_network_socket 2018-09-03 6:02 ` mituharu @ 2018-09-04 23:19 ` Noam Postavsky 2018-09-05 23:50 ` YAMAMOTO Mitsuharu 0 siblings, 1 reply; 8+ messages in thread From: Noam Postavsky @ 2018-09-04 23:19 UTC (permalink / raw) To: mituharu; +Cc: 32604 Looks good to me; a couple of minor suggestions below. mituharu@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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#32604: 26.1.50; memory leak in connect_network_socket 2018-09-04 23:19 ` Noam Postavsky @ 2018-09-05 23:50 ` YAMAMOTO Mitsuharu 2018-09-06 0:04 ` Noam Postavsky 0 siblings, 1 reply; 8+ messages in thread From: YAMAMOTO Mitsuharu @ 2018-09-05 23:50 UTC (permalink / raw) To: Noam Postavsky; +Cc: 32604 On Wed, 05 Sep 2018 08:19:48 +0900, Noam Postavsky wrote: > > Looks good to me; a couple of minor suggestions below. > > mituharu@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@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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#32604: 26.1.50; memory leak in connect_network_socket 2018-09-05 23:50 ` YAMAMOTO Mitsuharu @ 2018-09-06 0:04 ` Noam Postavsky 2018-09-06 23:41 ` YAMAMOTO Mitsuharu 0 siblings, 1 reply; 8+ messages in thread From: Noam Postavsky @ 2018-09-06 0:04 UTC (permalink / raw) To: YAMAMOTO Mitsuharu; +Cc: 32604 YAMAMOTO Mitsuharu <mituharu@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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#32604: 26.1.50; memory leak in connect_network_socket 2018-09-06 0:04 ` Noam Postavsky @ 2018-09-06 23:41 ` YAMAMOTO Mitsuharu 0 siblings, 0 replies; 8+ messages in thread From: YAMAMOTO Mitsuharu @ 2018-09-06 23:41 UTC (permalink / raw) To: 32604-done On Thu, 06 Sep 2018 09:04:27 +0900, Noam Postavsky wrote: > YAMAMOTO Mitsuharu <mituharu@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@math.s.chiba-u.ac.jp ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-09-06 23:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-09-01 5:38 bug#32604: 26.1.50; memory leak in connect_network_socket YAMAMOTO Mitsuharu 2018-09-01 5:46 ` YAMAMOTO Mitsuharu 2018-09-02 17:55 ` Noam Postavsky 2018-09-03 6:02 ` mituharu 2018-09-04 23:19 ` Noam Postavsky 2018-09-05 23:50 ` YAMAMOTO Mitsuharu 2018-09-06 0:04 ` Noam Postavsky 2018-09-06 23:41 ` YAMAMOTO Mitsuharu
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).