[kazu@iij.ad.jp: a bug of read-passwd]

[kazu@iij.ad.jp: a bug of read-passwd]

[Richard Stallman]

I wonder if this is a consequence of the new sit-for code.
Would someone please investigate, then ack this message?

------- Start of forwarded message -------
DKIM-Signature: a=rsa-sha1; c=relaxed/simple; d=iij.ad.jp; s=omgo0;
	t=1152939160; bh=3mhCZUjonim4IiCzzKXwlGTyEu8=; h=Received:Received:
	Date:Message-Id:To:Subject:From:X-Mailer:Mime-Version:Content-Type:
	Content-Transfer-Encoding; b=NYBNO/qQoOsroMzwkHp6O7uqWNe+Vhjlofn0m1
	CrNZrspQa6+N/HXDG3p5UepfAwt+dX1CtHd3Dxc8zuWmqePgbUq2wdLE+OFSdUW6F2W
	qN4QWThGMEPiM599VkzY8FDelF5emurJNv1Yqvyjwq1izfE39MKa6LLnB6ADVMVK4A=
Date: Sat, 15 Jul 2006 13:49:49 +0900 (JST)
To: emacs-devel@gnu.org
From: Kazu Yamamoto (=?iso-2022-jp?B?GyRCOzNLXE9CSScbKEI=?=) <kazu@iij.ad.jp>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Subject: a bug of read-passwd
X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed 
	version=3.0.4

Hello,

Recent changes of Emacs introduces a new bug to read-passwd.  If
read-passwd is called a filter of an asynchronous process and the
parent code executes sit-for, the cursor goes away from the minibuffer
and we cannot input our password.

The following code can reproduce this:

(defvar my-check nil)

(defun my-filter (process string)
  (set-buffer (process-buffer process))
  (read-passwd "Password: ")
  (setq my-check nil))

(let ((pro (start-process "*sh*" (current-buffer) "/bin/sh")))
  (setq my-check t)
  (set-process-filter pro 'my-filter)
  (while my-check
    (sit-for 0.1)
    (discard-input)))

The parent code calls sit-for to synchronize with its child process.
The code above is essence of a program which I use everyday.

This code works Emacs, at least, before Jun 31 2006.

- --Kazu Yamamoto


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel
------- End of forwarded message -------

Re: [kazu@iij.ad.jp: a bug of read-passwd]

[Chong Yidong]

> Recent changes of Emacs introduces a new bug to read-passwd.  If
> read-passwd is called a filter of an asynchronous process and the
> parent code executes sit-for, the cursor goes away from the minibuffer
> and we cannot input our password.
>
> The following code can reproduce this:
>
> (defvar my-check nil)
>
> (defun my-filter (process string)
>   (set-buffer (process-buffer process))
>   (read-passwd "Password: ")
>   (setq my-check nil))
>
> (let ((pro (start-process "*sh*" (current-buffer) "/bin/sh")))
>   (setq my-check t)
>   (set-process-filter pro 'my-filter)
>   (while my-check
>     (sit-for 0.1)
>     (discard-input)))

Does the code work if you remove the (discard-input)?

Re: [kazu@iij.ad.jp: a bug of read-passwd]

[Kazu Yamamoto]

Hello,

> > Recent changes of Emacs introduces a new bug to read-passwd.  If
> > read-passwd is called a filter of an asynchronous process and the
> > parent code executes sit-for, the cursor goes away from the minibuffer
> > and we cannot input our password.
>
> Does the code work if you remove the (discard-input)?

Yes.

I don't remember correctly but old Emacs requires (discard-input).

--Kazu

Re: a bug of read-passwd

[Kazu Yamamoto]

Hello,

> > > Recent changes of Emacs introduces a new bug to read-passwd.  If
> > > read-passwd is called a filter of an asynchronous process and the
> > > parent code executes sit-for, the cursor goes away from the minibuffer
> > > and we cannot input our password.
> >
> > Does the code work if you remove the (discard-input)?
> 
> Yes.

With the current CVS tree, this bug still exists and the answer
changed to "NO". 

--Kazu Yamamoto

Re: a bug of read-passwd

[Chong Yidong]

Kazu Yamamoto (山本和彦) <kazu@iij.ad.jp> writes:

> Hello,
>
>> > > Recent changes of Emacs introduces a new bug to read-passwd.  If
>> > > read-passwd is called a filter of an asynchronous process and the
>> > > parent code executes sit-for, the cursor goes away from the minibuffer
>> > > and we cannot input our password.
>> >
>> > Does the code work if you remove the (discard-input)?
>> 
>> Yes.
>
> With the current CVS tree, this bug still exists and the answer
> changed to "NO". 

I can reproduce this too.  I'll look into it.  Clearly, the
read-passwd in the asynchronous filter function is interfering with
the sit-for.

BTW, probably the cleaner thing to do for your purposes is to use
`accept-process-output':

(defvar my-check nil)

(defun my-filter (process string)
  (set-buffer (process-buffer process))
  (read-passwd "Password: ")
  (setq my-check nil))

(let ((pro (start-process "*sh*" (current-buffer) "/bin/sh")))
  (setq my-check t)
  (set-process-filter pro 'my-filter)
  (accept-process-output pro)
  (kill-process pro))

Re: a bug of read-passwd

[Chong Yidong]

>> (defvar my-check nil)
>>
>> (defun my-filter (process string)
>>   (set-buffer (process-buffer process))
>>   (read-passwd "Password: ")
>>   (setq my-check nil))
>>
>> (let ((pro (start-process "*sh*" (current-buffer) "/bin/sh")))
>>   (setq my-check t)
>>   (set-process-filter pro 'my-filter)
>>   (while my-check
>>     (sit-for 0.1)
>>     (discard-input)))
>
> I can reproduce this too.  I'll look into it.

The problem here is that the new sit-for calls (read-event) to wait
for new input.  During this time, the process filter takes over, and
that calls (read-passwd).  While the filter is waiting for the
password prompt, the sit-for timer expires, which causes a throw back
to the `sit-for', spoiling the `read-passwd'.

Not sure how to handle this situation (though, as I mentioned earlier,
the proper thing to do in this situation is to run
`accept-process-output').

Re: a bug of read-passwd

[Richard Stallman]

    The problem here is that the new sit-for calls (read-event) to wait
    for new input.  During this time, the process filter takes over, and
    that calls (read-passwd).  While the filter is waiting for the
    password prompt, the sit-for timer expires, which causes a throw back
    to the `sit-for', spoiling the `read-passwd'.

We would want sit-for's timer to be suspended in its operation while
the filter is running.  This should be the case for _all_ filters.  It
does not matter whether the filter does keyboard input.  If the filter
waits for process input, that too will run timers, and the same bug
can occur.

One solution is to set up a mechanism for the sit-for timer
to determine that it has been called inside a filter.
Then it needs to be able to set up for a certain function to be
called when the filter returns.

Another solution would be, don't use a timer.
read-event works by calling wait_reading_process_output.
It should be feasible to give it a new argument
which specifies a maximum time delay.

I think the latter method is cleaner.
What do you think?

Re: a bug of read-passwd

[Kazu Yamamoto]

Hello,

> Not sure how to handle this situation (though, as I mentioned earlier,
> the proper thing to do in this situation is to run
> `accept-process-output').

I changed my code so that it uses accept-process-output instead of
both sit-for and discard-input. I can type my password now. But the
code was stalled after that.

--Kazu Yamamoto

Re: a bug of read-passwd

[Chong Yidong]

> Another solution would be, don't use a timer.
> read-event works by calling wait_reading_process_output.
> It should be feasible to give it a new argument
> which specifies a maximum time delay.
>
> I think the latter method is cleaner.
> What do you think?

I think this is a good idea.  I'll look into implementing it.

Re: a bug of read-passwd

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> One solution is to set up a mechanism for the sit-for timer
> to determine that it has been called inside a filter.
> Then it needs to be able to set up for a certain function to be
> called when the filter returns.
>
> Another solution would be, don't use a timer.
> read-event works by calling wait_reading_process_output.
> It should be feasible to give it a new argument
> which specifies a maximum time delay.

OK, I looked into this.  I think that fixing read-event to give it a
new argument specifying a max delay turns out to be not simple to
implement.

(The detailed situation is this: the old sit-for called
wait_reading_process_output for a given duration, but that's
problematic since wait_reading_process_output can return due to fake
events.  The new sit-for solves this by calling read-event, which
relies on read_char, which loops calling wait_reading_process_output
with zero duration and discarding fake events [using
kbd_buffer_get_event, which does some complicated processing to figure
out if the event is fake or not].  The new sit-for interrupts this
loop using a timer.  If we want to hack read_char to accept a new arg
specifying the max delay, there would be two sets of timing code:
inside read_char, and inside wait_reading_process_output.  This is
very inelegant.)

Another possibility is to implement a `inhibit-timers' variable and
set it to t by default inside filter functions (similar to how
`inhibit-quit' is bound to t in filter functions.)  This is probably
easier to implement.  What do you think?

Re: a bug of read-passwd

[Stefan Monnier]

> Another possibility is to implement a `inhibit-timers' variable and
> set it to t by default inside filter functions (similar to how
> `inhibit-quit' is bound to t in filter functions.)  This is probably
> easier to implement.  What do you think?

I think it's a good idea, although let-binding timer-list (and
timer-idle-list) to nil might be even better since it'd still allow filter
functions to use timers.

BTW, something similar would be useful for edebug, when debugging
repeated timers.


        Stefan

Re: a bug of read-passwd

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Another possibility is to implement a `inhibit-timers' variable and
>> set it to t by default inside filter functions (similar to how
>> `inhibit-quit' is bound to t in filter functions.)  This is probably
>> easier to implement.  What do you think?
>
> I think it's a good idea, although let-binding timer-list (and
> timer-idle-list) to nil might be even better since it'd still allow filter
> functions to use timers.

Doing a let-bind is conceptually cleaner, but one can imagine a
situation in which a filter function needs access to a
previously-defined timer.  A `inhibit-timer' mechanism would allow
filter functions to manually turn the timer back on if they need to.

Re: a bug of read-passwd

[Stefan Monnier]

>>> Another possibility is to implement a `inhibit-timers' variable and
>>> set it to t by default inside filter functions (similar to how
>>> `inhibit-quit' is bound to t in filter functions.)  This is probably
>>> easier to implement.  What do you think?
>> 
>> I think it's a good idea, although let-binding timer-list (and
>> timer-idle-list) to nil might be even better since it'd still allow filter
>> functions to use timers.

> Doing a let-bind is conceptually cleaner, but one can imagine a
> situation in which a filter function needs access to a
> previously-defined timer.  A `inhibit-timer' mechanism would allow
> filter functions to manually turn the timer back on if they need to.

Agreed.  So maybe the inhibit-timer should not be boolean, but contain a list
of inhibited timers (it'd be set just before running a filter to the list
of timers active when the filter is started).


        Stefan

Re: a bug of read-passwd

[Richard Stallman]

    OK, I looked into this.  I think that fixing read-event to give it a
    new argument specifying a max delay turns out to be not simple to
    implement.

I think it is not so hard.

    (The detailed situation is this: the old sit-for called
    wait_reading_process_output for a given duration, but that's
    problematic since wait_reading_process_output can return due to fake
    events.  The new sit-for solves this by calling read-event, which
    relies on read_char, which loops calling wait_reading_process_output
    with zero duration and discarding fake events [using
    kbd_buffer_get_event, which does some complicated processing to figure
    out if the event is fake or not].

It would work right if sit-for passes down to wait_reading_process_output
the fact that it has a timeout.


				       The new sit-for interrupts this
    loop using a timer.  If we want to hack read_char to accept a new arg
    specifying the max delay, there would be two sets of timing code:
    inside read_char, and inside wait_reading_process_output.

read_char needs to do arithmetic so it can recompute the remaining
timeout, in case wait_reading_process_output returned an event
which got discarded.  But this is not very hard.  Just add the
specified timeout to the current time, at the start, and subtract
the current time before each call to wait_reading_process_output.

wait_reading_process_output does this same computation in order
to call select with the proper remaining timeout.

The inelegance of this is just a matter of repeating a little code.
At the deeper level, this design is elegant because it is very safe.
So I think we should try it.

The superficial inelegance can be removed easily if
wait_reading_process_output accepted an ending time, rather than a
duration.  That way, the addition would only be done once at the
outset, and the subtraction would only have to be done when
wait_reading_process_output is about to call select.

Re: a bug of read-passwd

[Chong Yidong]

> read_char needs to do arithmetic so it can recompute the remaining
> timeout, in case wait_reading_process_output returned an event
> which got discarded.  But this is not very hard.  Just add the
> specified timeout to the current time, at the start, and subtract
> the current time before each call to wait_reading_process_output.
>
> wait_reading_process_output does this same computation in order
> to call select with the proper remaining timeout.
>
> The inelegance of this is just a matter of repeating a little code.
> At the deeper level, this design is elegant because it is very safe.
> So I think we should try it.

I've checked in changes that implement this.  `read-event' and friends
now have an optional third argument that tells them how long to wait
for.

Re: a bug of read-passwd

[Richard Stallman]

    I've checked in changes that implement this.  `read-event' and friends
    now have an optional third argument that tells them how long to wait
    for.

Thanks.  Did it fix the bugs that had been seen?

(Don't forget to update the Lisp manual and etc/NEWS.)

Re: a bug of read-passwd

[Yidong Chong]

>    I've checked in changes that implement this.  `read-event' and friends
>    now have an optional third argument that tells them how long to wait
>    for.
>
> Thanks.  Did it fix the bugs that had been seen?

Yes.

> (Don't forget to update the Lisp manual and etc/NEWS.)

Done.

Re: a bug of read-passwd

[Kazu Yamamoto]

Hello,

>     I've checked in changes that implement this.  `read-event' and friends
>     now have an optional third argument that tells them how long to wait
>     for.
> 
> Thanks.  Did it fix the bugs that had been seen?

Yes. Thanks!

--Kazu Yamamoto