r-devel / r-dev-day

Repo to organize tasks for R Dev Days
7 stars 1 forks source link

Memory leak when redirecting messages with `sink()` #30

Closed georgestagg closed 3 weeks ago

georgestagg commented 1 month ago

In webR I make a patch to the C function do_sink(), IIRC to avoid a minor memory leak.

Below I’ve pasted a copy of the rationale that I wrote at the time. We could spend some time to check my logic, and if the issue is genuine submit a patch upstream.


Consider the following code that temporarily redirects stderr messages to a file using sink(), first setting then removing the redirection:

zz <- file("/tmp/tmp.err", open = "wt")
sink(zz, type = "message")
stop("foo")
sink(type = "message")

Looking at the source for sink(), when type = "message" it eventually runs

.Internal(sink(file, FALSE, TRUE, FALSE))

In the first call to sink() file is passed in as the connection zz. On the second call the file argument is NULL, and so R sets it to stderr() instead (line 24).

Now, inside the associated do_sink() function the first argument is converted into an integer:

icon = asInteger(CAR(args));

Then, later (line 5359), if icon > 0 the error connection is redirected to that integer and an external pointer is preserved, and if icon < 0 the original error connection is restored and that external pointer is released. So, after redirection and release things should balance.

However, I claim that the branch that releases the external pointer never runs, because icon is not less than zero on the second invocation of sink(), because the integer value of stderr() is 2:

> as.integer(zz)
[1] 3
> as.integer(stderr())
[1] 2
georgestagg commented 1 month ago

Bug report submitted to bugzilla, 18760.

hturner commented 3 weeks ago

A fix has since been made to close this bug.