neovim/neovim — pull request #1350 by splinterofchaos (2014-10-27 03:51:44+00:00)
Title: [RFC] valgrind/jobs: Fix invalid reads.

I've been having trouble with #1255 over memory issues, so... long story short, I have a simple test script:
```vim
function! s:cat()
if v:job_data[1] == 'stdout'
let @a = v:job_data[2]
put a
elseif v:job_data[1] == 'exit'
put =string('exit')
end
endfunction

au! JobActivity cat call s:cat()
```
which I invoke with `:call jobstart('cat', 'cat', ['src/nvim/eval.c'])` and valgrind gives a very long report on the master branch of various errors involving `rstream`. Here's one:
```
==9518== Invalid read of size 8
==9518== at 0x5F6821: rstream_pending (rstream.c:298)
==9518== by 0x4BA227: on_job_event (eval.c:19566)
==9518== by 0x5F93DA: event_process (event.c:133)
==9518== by 0x598815: nv_event (normal.c:7379)
==9518== by 0x58A899: normal_cmd (normal.c:915)
==9518== by 0x4F0E3F: main_loop (main.c:751)
==9518== by 0x4F07ED: main (main.c:536)
==9518== Address 0x7982fc0 is 32 bytes inside a block of size 80 free'd
==9518== at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9518== by 0x5F65BC: rstream_free (rstream.c:207)
==9518== by 0x5F899F: close_cb (job.c:454)
==9518== by 0x605957: uv__finish_close (core.c:218)
==9518== by 0x605957: uv__run_closing_handles (core.c:232)
==9518== by 0x605957: uv_run (core.c:300)
==9518== by 0x5F94B2: loop (event.c:155)
==9518== by 0x5F92EE: event_poll (event.c:100)
==9518== by 0x5F56FF: input_poll (input.c:170)
==9518== by 0x5F55D7: os_breakcheck (input.c:134)
==9518== by 0x58857F: ui_breakcheck (ui.c:216)
==9518== by 0x45A7C4: line_breakcheck (misc1.c:3380)
==9518== by 0x5BD833: auto_next_pat (fileio.c:6874)
==9518== by 0x5BD22E: apply_autocmds_group (fileio.c:6702)
==9518== by 0x5BCAAD: apply_autocmds (fileio.c:6352)
==9518== by 0x4BA378: apply_job_autocmds (eval.c:19594)
==9518== by 0x4BA295: on_job_event (eval.c:19572)
==9518== by 0x5F93DA: event_process (event.c:133)
==9518== by 0x598815: nv_event (normal.c:7379)
==9518== by 0x58A899: normal_cmd (normal.c:915)
==9518== by 0x4F0E3F: main_loop (main.c:751)
==9518== by 0x4F07ED: main (main.c:536)
```
Generated with `$ valgrind --leak-check=full --track-origins=yes --num-callers=50 --log-file=vlog build/bin/nvim`.

So, we have `on_job_event()` leading to the uv event loop calling `close_cb()` and freeing the stream, and some time later another `on_job_event()` tries to read the free'd stream. Freeing the `rstream` when closing might be too soon; plus I think that the process might be ending before `[rw]steam` has flushed. I delayed this until `refcount == 0`.

The only non-rstream valgrind error I encountered:
```
==15694== Invalid read of size 8
==15694== at 0x5F86C8: job_data (job.c:381)
==15694== by 0x4BA2F0: on_job_event (eval.c:19573)
==15694== by 0x5F93E8: event_process (event.c:133)
==15694== by 0x598895: nv_event (normal.c:7379)
==15694== by 0x58A919: normal_cmd (normal.c:915)
==15694== by 0x4F0EBF: main_loop (main.c:751)
==15694== by 0x4F086D: main (main.c:536)
==15694== Address 0x79d68b0 is 16 bytes after a block of size 16 in arena "client"
```
I believe this is the exit callback since it doesn't seem to complain about accessing the `rstream`. We can see from `eval.c`:

```c
static void on_job_exit(Job *job, void *data)
{
push_job_event(data, NULL, "exit");
}
```
This is called by `job_exit_callback()` only moments before freeing the whole job structure and no event loop runs in between. After calling `job_exit_callback()`, instead of freeing the job, I made it an event and made it call `push_event()` so that it would run after the event `on_job_exit()` pushed.

Finally, the exit callback never seems to run, and I think making the first parameter of `push_job_event()` `data` instead of `job` a typo, so I fixed that too. (Only after I made that change did the `v:job_data[1] == 'exit'` branch ever run.)