* Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0 [not found] <E1PqVCx-0003xM-4I@vcs-noshell.in.savannah.gnu.org> @ 2011-02-21 20:50 ` Ludovic Courtès 2011-02-21 21:19 ` Andy Wingo 0 siblings, 1 reply; 5+ messages in thread From: Ludovic Courtès @ 2011-02-21 20:50 UTC (permalink / raw) To: Andy Wingo; +Cc: guile-devel Hi! "Andy Wingo" <wingo@pobox.com> writes: > fix a couple leaks in ports.c. thanks valgrind! > > * libguile/ports.c (scm_i_remove_port): Fix a case in which ports > explictly closed via close-port would leak their iconv_t data. > (scm_set_port_encoding_x): scm_i_set_port_encoding_x strdups its > argument, so we need to free the locale encoding of the incoming str. Good catch! > --- a/libguile/ports.c > +++ b/libguile/ports.c > @@ -661,6 +661,19 @@ scm_i_remove_port (SCM port) > scm_port_non_buffer (p); > p->putback_buf = NULL; > p->putback_buf_size = 0; > + > + if (p->input_cd != (iconv_t) -1) > + { > + iconv_close (p->input_cd); > + p->input_cd = (iconv_t) -1; > + } > + > + if (p->output_cd != (iconv_t) -1) > + { > + iconv_close (p->output_cd); > + p->output_cd = (iconv_t) -1; > + } > + I don’t think this is needed: each port has a finalizer, ‘finalize_port’, which normally takes care of this, eventually. Thanks, Ludo’. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0 2011-02-21 20:50 ` [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0 Ludovic Courtès @ 2011-02-21 21:19 ` Andy Wingo 2011-02-22 11:36 ` Ludovic Courtès 0 siblings, 1 reply; 5+ messages in thread From: Andy Wingo @ 2011-02-21 21:19 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel On Mon 21 Feb 2011 21:50, ludo@gnu.org (Ludovic Courtès) writes: >> --- a/libguile/ports.c >> +++ b/libguile/ports.c >> @@ -661,6 +661,19 @@ scm_i_remove_port (SCM port) >> scm_port_non_buffer (p); >> p->putback_buf = NULL; >> p->putback_buf_size = 0; >> + >> + if (p->input_cd != (iconv_t) -1) >> + { >> + iconv_close (p->input_cd); >> + p->input_cd = (iconv_t) -1; >> + } >> + >> + if (p->output_cd != (iconv_t) -1) >> + { >> + iconv_close (p->output_cd); >> + p->output_cd = (iconv_t) -1; >> + } >> + > > I don’t think this is needed: each port has a finalizer, > ‘finalize_port’, which normally takes care of this, eventually. It is needed, but only in the case that you `close-port' explicitly. The block in finalize_port only takes care of gc'd open ports. Regards, Andy -- http://wingolog.org/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0 2011-02-21 21:19 ` Andy Wingo @ 2011-02-22 11:36 ` Ludovic Courtès 2011-02-22 19:11 ` Andy Wingo 0 siblings, 1 reply; 5+ messages in thread From: Ludovic Courtès @ 2011-02-22 11:36 UTC (permalink / raw) To: Andy Wingo; +Cc: guile-devel Hello, Andy Wingo <wingo@pobox.com> writes: > On Mon 21 Feb 2011 21:50, ludo@gnu.org (Ludovic Courtès) writes: > >>> --- a/libguile/ports.c >>> +++ b/libguile/ports.c >>> @@ -661,6 +661,19 @@ scm_i_remove_port (SCM port) >>> scm_port_non_buffer (p); >>> p->putback_buf = NULL; >>> p->putback_buf_size = 0; >>> + >>> + if (p->input_cd != (iconv_t) -1) >>> + { >>> + iconv_close (p->input_cd); >>> + p->input_cd = (iconv_t) -1; >>> + } >>> + >>> + if (p->output_cd != (iconv_t) -1) >>> + { >>> + iconv_close (p->output_cd); >>> + p->output_cd = (iconv_t) -1; >>> + } >>> + >> >> I don’t think this is needed: each port has a finalizer, >> ‘finalize_port’, which normally takes care of this, eventually. > > It is needed, but only in the case that you `close-port' explicitly. > The block in finalize_port only takes care of gc'd open ports. Right. Closed ports are eventually GC’d, so in that sense it is not strictly needed, but OK. Valgrind was wrong! ;-) Ludo’. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0 2011-02-22 11:36 ` Ludovic Courtès @ 2011-02-22 19:11 ` Andy Wingo 2011-02-22 20:35 ` Ludovic Courtès 0 siblings, 1 reply; 5+ messages in thread From: Andy Wingo @ 2011-02-22 19:11 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel On Tue 22 Feb 2011 12:36, ludo@gnu.org (Ludovic Courtès) writes: >>>> + >>>> + if (p->input_cd != (iconv_t) -1) >>>> + { >>>> + iconv_close (p->input_cd); >>>> + p->input_cd = (iconv_t) -1; >>>> + } >>>> + >>>> + if (p->output_cd != (iconv_t) -1) >>>> + { >>>> + iconv_close (p->output_cd); >>>> + p->output_cd = (iconv_t) -1; >>>> + } >>>> + >>> >>> I don’t think this is needed: each port has a finalizer, >>> ‘finalize_port’, which normally takes care of this, eventually. >> >> It is needed, but only in the case that you `close-port' explicitly. >> The block in finalize_port only takes care of gc'd open ports. > > Right. Closed ports are eventually GC’d, so in that sense it is not > strictly needed, but OK. > > Valgrind was wrong! ;-) You are setting yourself up for a fall here ;) When you close a port via "close-port", you remove the port's SCM_PTAB_ENTRY (port). The SCM_PTAB_ENTRY points to the iconv_t, so the finalizer would not have a chance to free it, because it can't get to it. You need to free the iconv_t at the time you remove the link from the port to the SCM_PTAB_ENTRY -- i.e. at close-port time. Otherwise you leak the iconv_t. Trust me: about 500K requests into meta/guile examples/web/hello.scm, the memory usage was up at about a gigabyte or so :) Andy -- http://wingolog.org/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0 2011-02-22 19:11 ` Andy Wingo @ 2011-02-22 20:35 ` Ludovic Courtès 0 siblings, 0 replies; 5+ messages in thread From: Ludovic Courtès @ 2011-02-22 20:35 UTC (permalink / raw) To: Andy Wingo; +Cc: guile-devel Hi! Andy Wingo <wingo@pobox.com> writes: > On Tue 22 Feb 2011 12:36, ludo@gnu.org (Ludovic Courtès) writes: > >>>>> + >>>>> + if (p->input_cd != (iconv_t) -1) >>>>> + { >>>>> + iconv_close (p->input_cd); >>>>> + p->input_cd = (iconv_t) -1; >>>>> + } >>>>> + >>>>> + if (p->output_cd != (iconv_t) -1) >>>>> + { >>>>> + iconv_close (p->output_cd); >>>>> + p->output_cd = (iconv_t) -1; >>>>> + } >>>>> + >>>> >>>> I don’t think this is needed: each port has a finalizer, >>>> ‘finalize_port’, which normally takes care of this, eventually. >>> >>> It is needed, but only in the case that you `close-port' explicitly. >>> The block in finalize_port only takes care of gc'd open ports. >> >> Right. Closed ports are eventually GC’d, so in that sense it is not >> strictly needed, but OK. >> >> Valgrind was wrong! ;-) > > You are setting yourself up for a fall here ;) Damn, indeed! > When you close a port via "close-port", you remove the port's > SCM_PTAB_ENTRY (port). The SCM_PTAB_ENTRY points to the iconv_t, so the > finalizer would not have a chance to free it, because it can't get to > it. You need to free the iconv_t at the time you remove the link from > the port to the SCM_PTAB_ENTRY -- i.e. at close-port time. Otherwise > you leak the iconv_t. > > Trust me: about 500K requests into meta/guile examples/web/hello.scm, > the memory usage was up at about a gigabyte or so :) Ouch. Thanks for the explanation, I had clearly overlooked this. Ludo’. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-02-22 20:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <E1PqVCx-0003xM-4I@vcs-noshell.in.savannah.gnu.org>
2011-02-21 20:50 ` [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.0-4-g3e05fc0 Ludovic Courtès
2011-02-21 21:19 ` Andy Wingo
2011-02-22 11:36 ` Ludovic Courtès
2011-02-22 19:11 ` Andy Wingo
2011-02-22 20:35 ` Ludovic Courtès
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).