From: Johannes Sixt <j6t@kdbg.org>
Date: Thu, 22 Dec 2016 22:28:29 GMT
Subject: Re: [PATCH v2 3/3] mingw: replace isatty() hack
Message-ID: <04859cf9-e67a-28ab-ccb3-249687e696c8@kdbg.org>
In-Reply-To: <alpine.DEB.2.20.1612222235200.155951@virtualbox>

Am 22.12.2016 um 22:37 schrieb Johannes Schindelin:
> Hi Hannes,
>
> On Thu, 22 Dec 2016, Johannes Sixt wrote:
>
>> Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:
>>> +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
>>> +{
>>> +	/*
>>> +	 * Create a copy of the original handle associated with fd
>>> +	 * because the original will get closed when we dup2().
>>> +	 */
>>> +	HANDLE handle = (HANDLE)_get_osfhandle(fd);
>>> +	HANDLE duplicate = duplicate_handle(handle);
>>>
>>> +	/* Create a temp fd associated with the already open "new_handle". */
>>> +	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
>>>
>>> +	assert((fd == 1) || (fd == 2));
>>>
>>> +	/*
>>> +	 * Use stock dup2() to re-bind fd to the new handle.  Note that
>>> +	 * this will implicitly close(1) and close both fd=1 and the
>>> +	 * originally associated handle.  It will open a new fd=1 and
>>> +	 * call DuplicateHandle() on the handle associated with new_fd.
>>> +	 * It is because of this implicit close() that we created the
>>> +	 * copy of the original.
>>> +	 *
>>> +	 * Note that the OS can recycle HANDLE (numbers) just like it
>>> +	 * recycles fd (numbers), so we must update the cached value
>>> +	 * of "console".  You can use GetFileType() to see that
>>> +	 * handle and _get_osfhandle(fd) may have the same number
>>> +	 * value, but they refer to different actual files now.
>>
>> Certainly, the OS does not recycle handle values that are in use (open). Then
>> I do not quite get the point of this paragraph. See...
>>
>>> +	 *
>>> +	 * Note that dup2() when given target := {0,1,2} will also
>>> +	 * call SetStdHandle(), so we don't need to worry about that.
>>> +	 */
>>> +	dup2(new_fd, fd);
>>> +	if (console == handle)
>>> +		console = duplicate;
>>
>> ... This is where "the cached value of console is updated", right? If console
>> == handle, then this is not because a handle value was recycled, but because
>> fd *was* console. Since the old value of console has been closed by the
>> dup2(), we must refer to the back-up value in the future. Am I missing
>> something?
>
> You are correct, we must update `console` because `handle` is no longer
> the handle we want.
>
> The comment above only meant to reinforce that we have to forget about the
> previous handle, too, as we might access something completely different
> than a console otherwise.

It is like accessing a pointer after free().

When I read the paragraph for the first time, I expected some 
information to be saved, then some handles to be closed and re-opened, 
which would possibly recycle a handle value, and then same state to be 
resurrected depending on the information saved earlier.

But nothing of this kind happens, only:

	dup2(new_fd, fd);
	if (console == handle)
		console = duplicate;

which is necessary and, once one has understood the context, obvious.

> Would you have a suggestion how to rephrase the comment to make it less
> confusing?

Perhaps

	 * This might close the cached console handle.
	 * We must cache the live duplicate instead.

Then update the cache before the dup2, because at this time all 3 of 
console, handle, and duplicate are live and cannot be recycled:

	if (console == handle)
		console = duplicate;
	dup2(new_fd, fd);

-- Hannes