On Mon, Feb 29, 2016 at 1:01 PM, Junio C Hamano <gits...@pobox.com> wrote: > Johannes Sixt <j...@kdbg.org> writes: > >> The culprit seems to be default_task_finished(), which accesses argv[] >> of the struct child_process after finish_command has released it, >> provided the child exited with an error, for example: > > Thanks for a report. > >> ==3395== Invalid read of size 8 >> ==3395== at 0x54F991: default_task_finished (run-command.c:932) > > That one and also start_failure() do run after child_process_clear() > has cleaned things up, so they shouldn't be looking at argv[] (or > anything in that structure for that matter).
I am undecided if easy access to the child process struct is a benefit or not for the callback. It makes error reporting easy, but maybe hacky as you poke around with the argv. Maybe we want to remove the struct child_process from the function signature of the callbacks and callbacks need to rely on the data provided solely thru the pointer as passed around for callback purposes, which the user is free to use for any kind of data. As a rather quickfix for 2.8 (and 2.7) we could however just breakup the finish_command function and call child_process_clear after the callbacks have run. > > > >> ==3395== by 0x49158F: update_clone_task_finished (submodule--helper.c:421) >> ==3395== by 0x5504A2: pp_collect_finished (run-command.c:1122) >> ==3395== by 0x5507C7: run_processes_parallel (run-command.c:1194) >> ==3395== by 0x4918EB: update_clone (submodule--helper.c:483) >> ==3395== by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527) >> ==3395== by 0x405CBE: run_builtin (git.c:353) >> ==3395== by 0x405EAA: handle_builtin (git.c:540) >> ==3395== by 0x405FCC: run_argv (git.c:594) >> ==3395== by 0x4061BF: main (git.c:701) >> ==3395== Address 0x5e49370 is 0 bytes inside a block of size 192 free'd >> ==3395== at 0x4C2A37C: free (in >> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==3395== by 0x4A26EE: argv_array_clear (argv-array.c:73) >> ==3395== by 0x54DFC4: child_process_clear (run-command.c:18) >> ==3395== by 0x54EFA7: finish_command (run-command.c:539) >> ==3395== by 0x550413: pp_collect_finished (run-command.c:1120) >> ==3395== by 0x5507C7: run_processes_parallel (run-command.c:1194) >> ==3395== by 0x4918EB: update_clone (submodule--helper.c:483) >> ==3395== by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527) >> ==3395== by 0x405CBE: run_builtin (git.c:353) >> ==3395== by 0x405EAA: handle_builtin (git.c:540) >> ==3395== by 0x405FCC: run_argv (git.c:594) >> ==3395== by 0x4061BF: main (git.c:701) >> >> I haven't thought about a solution, yet. Perhaps you have ideas. >> >> -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
