To: vim_dev@googlegroups.com Subject: Patch 7.4.1717 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ Patch 7.4.1717 Problem: Leaking memory when opening a channel fails. Solution: Unreference partials in job options. Files: src/eval.c, src/channel.c, src/proto/channel.pro, src/testdir/test_channel.vim *** ../vim-7.4.1716/src/eval.c 2016-04-06 22:59:33.503226978 +0200 --- src/eval.c 2016-04-07 21:15:46.514517082 +0200 *************** *** 10321,10329 **** return; clear_job_options(&opt); if (get_job_options(&argvars[1], &opt, ! JO_CB_ALL + JO_TIMEOUT_ALL + JO_MODE_ALL) == FAIL) ! return; ! channel_set_options(channel, &opt); } /* --- 10321,10329 ---- return; clear_job_options(&opt); if (get_job_options(&argvars[1], &opt, ! JO_CB_ALL + JO_TIMEOUT_ALL + JO_MODE_ALL) == OK) ! channel_set_options(channel, &opt); ! free_job_options(&opt); } /* *************** *** 14889,14897 **** if (job == NULL) return; clear_job_options(&opt); ! if (get_job_options(&argvars[1], &opt, JO_STOPONEXIT + JO_EXIT_CB) == FAIL) ! return; ! job_set_options(job, &opt); } /* --- 14889,14897 ---- if (job == NULL) return; clear_job_options(&opt); ! if (get_job_options(&argvars[1], &opt, JO_STOPONEXIT + JO_EXIT_CB) == OK) ! job_set_options(job, &opt); ! free_job_options(&opt); } /* *** ../vim-7.4.1716/src/channel.c 2016-03-30 21:06:53.723772030 +0200 --- src/channel.c 2016-04-07 21:16:18.230184688 +0200 *************** *** 858,864 **** char *rest; int port; jobopt_T opt; ! channel_T *channel; address = get_tv_string(&argvars[0]); if (argvars[1].v_type != VAR_UNKNOWN --- 858,864 ---- char *rest; int port; jobopt_T opt; ! channel_T *channel = NULL; address = get_tv_string(&argvars[0]); if (argvars[1].v_type != VAR_UNKNOWN *************** *** 890,900 **** opt.jo_timeout = 2000; if (get_job_options(&argvars[1], &opt, JO_MODE_ALL + JO_CB_ALL + JO_WAITTIME + JO_TIMEOUT_ALL) == FAIL) ! return NULL; if (opt.jo_timeout < 0) { EMSG(_(e_invarg)); ! return NULL; } channel = channel_open((char *)address, port, opt.jo_waittime, NULL); --- 890,900 ---- opt.jo_timeout = 2000; if (get_job_options(&argvars[1], &opt, JO_MODE_ALL + JO_CB_ALL + JO_WAITTIME + JO_TIMEOUT_ALL) == FAIL) ! goto theend; if (opt.jo_timeout < 0) { EMSG(_(e_invarg)); ! goto theend; } channel = channel_open((char *)address, port, opt.jo_waittime, NULL); *************** *** 903,908 **** --- 903,910 ---- opt.jo_set = JO_ALL; channel_set_options(channel, &opt); } + theend: + free_job_options(&opt); return channel; } *************** *** 2897,2903 **** clear_job_options(&opt); if (get_job_options(&argvars[1], &opt, JO_TIMEOUT + JO_PART + JO_ID) == FAIL) ! return; channel = get_channel_arg(&argvars[0], TRUE); if (channel != NULL) --- 2899,2905 ---- clear_job_options(&opt); if (get_job_options(&argvars[1], &opt, JO_TIMEOUT + JO_PART + JO_ID) == FAIL) ! goto theend; channel = get_channel_arg(&argvars[0], TRUE); if (channel != NULL) *************** *** 2930,2935 **** --- 2932,2940 ---- } } } + + theend: + free_job_options(&opt); } # if defined(WIN32) || defined(FEAT_GUI_X11) || defined(FEAT_GUI_GTK) \ *************** *** 3056,3068 **** channel_T *channel; int part_send; channel = get_channel_arg(&argvars[0], TRUE); if (channel == NULL) return NULL; part_send = channel_part_send(channel); *part_read = channel_part_read(channel); - clear_job_options(opt); if (get_job_options(&argvars[2], opt, JO_CALLBACK + JO_TIMEOUT) == FAIL) return NULL; --- 3061,3073 ---- channel_T *channel; int part_send; + clear_job_options(opt); channel = get_channel_arg(&argvars[0], TRUE); if (channel == NULL) return NULL; part_send = channel_part_send(channel); *part_read = channel_part_read(channel); if (get_job_options(&argvars[2], opt, JO_CALLBACK + JO_TIMEOUT) == FAIL) return NULL; *************** *** 3145,3150 **** --- 3150,3156 ---- free_tv(listtv); } } + free_job_options(&opt); } /* *************** *** 3175,3180 **** --- 3181,3187 ---- timeout = channel_get_timeout(channel, part_read); rettv->vval.v_string = channel_read_block(channel, part_read, timeout); } + free_job_options(&opt); } # if (defined(UNIX) && !defined(HAVE_SELECT)) || defined(PROTO) *************** *** 3545,3550 **** --- 3552,3560 ---- return OK; } + /* + * Clear a jobopt_T before using it. + */ void clear_job_options(jobopt_T *opt) { *************** *** 3552,3557 **** --- 3562,3583 ---- } /* + * Free any members of a jobopt_T. + */ + void + free_job_options(jobopt_T *opt) + { + if (opt->jo_partial != NULL) + partial_unref(opt->jo_partial); + if (opt->jo_out_partial != NULL) + partial_unref(opt->jo_out_partial); + if (opt->jo_err_partial != NULL) + partial_unref(opt->jo_err_partial); + if (opt->jo_close_partial != NULL) + partial_unref(opt->jo_close_partial); + } + + /* * Get the PART_ number from the first character of an option name. */ static int *************** *** 4053,4058 **** --- 4079,4087 ---- return NULL; job->jv_status = JOB_FAILED; + #ifndef USE_ARGV + ga_init2(&ga, (int)sizeof(char*), 20); + #endif /* Default mode is NL. */ clear_job_options(&opt); *************** *** 4060,4066 **** if (get_job_options(&argvars[1], &opt, JO_MODE_ALL + JO_CB_ALL + JO_TIMEOUT_ALL + JO_STOPONEXIT + JO_EXIT_CB + JO_OUT_IO + JO_BLOCK_WRITE) == FAIL) ! return job; /* Check that when io is "file" that there is a file name. */ for (part = PART_OUT; part <= PART_IN; ++part) --- 4089,4095 ---- if (get_job_options(&argvars[1], &opt, JO_MODE_ALL + JO_CB_ALL + JO_TIMEOUT_ALL + JO_STOPONEXIT + JO_EXIT_CB + JO_OUT_IO + JO_BLOCK_WRITE) == FAIL) ! goto theend; /* Check that when io is "file" that there is a file name. */ for (part = PART_OUT; part <= PART_IN; ++part) *************** *** 4070,4076 **** || *opt.jo_io_name[part] == NUL)) { EMSG(_("E920: _io file requires _name to be set")); ! return job; } if ((opt.jo_set & JO_IN_IO) && opt.jo_io[PART_IN] == JIO_BUFFER) --- 4099,4105 ---- || *opt.jo_io_name[part] == NUL)) { EMSG(_("E920: _io file requires _name to be set")); ! goto theend; } if ((opt.jo_set & JO_IN_IO) && opt.jo_io[PART_IN] == JIO_BUFFER) *************** *** 4091,4097 **** else buf = buflist_find_by_name(opt.jo_io_name[PART_IN], FALSE); if (buf == NULL) ! return job; if (buf->b_ml.ml_mfp == NULL) { char_u numbuf[NUMBUFLEN]; --- 4120,4126 ---- else buf = buflist_find_by_name(opt.jo_io_name[PART_IN], FALSE); if (buf == NULL) ! goto theend; if (buf->b_ml.ml_mfp == NULL) { char_u numbuf[NUMBUFLEN]; *************** *** 4105,4121 **** else s = opt.jo_io_name[PART_IN]; EMSG2(_("E918: buffer must be loaded: %s"), s); ! return job; } job->jv_in_buf = buf; } job_set_options(job, &opt); - #ifndef USE_ARGV - ga_init2(&ga, (int)sizeof(char*), 20); - #endif - if (argvars[0].v_type == VAR_STRING) { /* Command is a string. */ --- 4134,4146 ---- else s = opt.jo_io_name[PART_IN]; EMSG2(_("E918: buffer must be loaded: %s"), s); ! goto theend; } job->jv_in_buf = buf; } job_set_options(job, &opt); if (argvars[0].v_type == VAR_STRING) { /* Command is a string. */ *************** *** 4123,4133 **** if (cmd == NULL || *cmd == NUL) { EMSG(_(e_invarg)); ! return job; } #ifdef USE_ARGV if (mch_parse_cmd(cmd, FALSE, &argv, &argc) == FAIL) ! return job; argv[argc] = NULL; #endif } --- 4148,4158 ---- if (cmd == NULL || *cmd == NUL) { EMSG(_(e_invarg)); ! goto theend; } #ifdef USE_ARGV if (mch_parse_cmd(cmd, FALSE, &argv, &argc) == FAIL) ! goto theend; argv[argc] = NULL; #endif } *************** *** 4136,4142 **** || argvars[0].vval.v_list->lv_len < 1) { EMSG(_(e_invarg)); ! return job; } else { --- 4161,4167 ---- || argvars[0].vval.v_list->lv_len < 1) { EMSG(_(e_invarg)); ! goto theend; } else { *************** *** 4148,4154 **** /* Pass argv[] to mch_call_shell(). */ argv = (char **)alloc(sizeof(char *) * (l->lv_len + 1)); if (argv == NULL) ! return job; #endif for (li = l->lv_first; li != NULL; li = li->li_next) { --- 4173,4179 ---- /* Pass argv[] to mch_call_shell(). */ argv = (char **)alloc(sizeof(char *) * (l->lv_len + 1)); if (argv == NULL) ! goto theend; #endif for (li = l->lv_first; li != NULL; li = li->li_next) { *************** *** 4222,4227 **** --- 4247,4253 ---- #else vim_free(ga.ga_data); #endif + free_job_options(&opt); return job; } *** ../vim-7.4.1716/src/proto/channel.pro 2016-03-28 19:16:15.669846492 +0200 --- src/proto/channel.pro 2016-04-07 21:16:38.017977325 +0200 *************** *** 46,51 **** --- 46,52 ---- ch_mode_T channel_get_mode(channel_T *channel, int part); int channel_get_timeout(channel_T *channel, int part); void clear_job_options(jobopt_T *opt); + void free_job_options(jobopt_T *opt); int get_job_options(typval_T *tv, jobopt_T *opt, int supported); channel_T *get_channel_arg(typval_T *tv, int check_open); void job_unref(job_T *job); *** ../vim-7.4.1716/src/testdir/test_channel.vim 2016-03-30 21:06:53.723772030 +0200 --- src/testdir/test_channel.vim 2016-04-07 21:07:30.883739433 +0200 *************** *** 1231,1235 **** --- 1231,1247 ---- call assert_fails('call job_start("")', 'E474:') endfunc + " This leaking memory. + func Test_partial_in_channel_cycle() + let d = {} + let d.a = function('string', [d]) + try + let d.b = ch_open('nowhere:123', {'close_cb': d.a}) + catch + call assert_exception('E901:') + endtry + unlet d + endfunc + " Uncomment this to see what happens, output is in src/testdir/channellog. " call ch_logfile('channellog', 'w') *** ../vim-7.4.1716/src/version.c 2016-04-06 23:06:18.590052451 +0200 --- src/version.c 2016-04-07 21:38:15.284413842 +0200 *************** *** 750,751 **** --- 750,753 ---- { /* Add new patch number below this line */ + /**/ + 1717, /**/ -- hundred-and-one symptoms of being an internet addict: 242. You turn down a better-paying job because it doesn't come with a free e-mail account. /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///