Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Stefan Monnier]

>     Attempt to avoid crashes in plist-member
    
>     * src/fns.c (Fplist_member): Don't call QUIT between a CONSP test
>     and a call to XCDR.  (Bug#21655)

I have no objection to the patch itself, but I must say that I wonder
how it can avoid crashes.  IOW why would calling QUIT between a CONSP test
and a call to XCDR be dangerous?


        Stefan

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 12 Oct 2015 21:07:00 -0400
> 
> >     Attempt to avoid crashes in plist-member
>     
> >     * src/fns.c (Fplist_member): Don't call QUIT between a CONSP test
> >     and a call to XCDR.  (Bug#21655)
> 
> I have no objection to the patch itself, but I must say that I wonder
> how it can avoid crashes.  IOW why would calling QUIT between a CONSP test
> and a call to XCDR be dangerous?

QUIT could call some Lisp.

How else would you explain a segfault in that loop?

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Paul Eggert]

Eli Zaretskii wrote:
> QUIT could call some Lisp.

Sure, but Lisp cannot affect the value of CONSP (plist).

> How else would you explain a segfault in that loop?

It's not clear the code is in that loop.  It could merely be 
RtlCaptureStackBackTrace misbehaving, no?

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 12 Oct 2015 20:36:06 -0700
> 
>     QUIT could call some Lisp.
>
> Sure, but Lisp cannot affect the value of CONSP (plist).

Not the value of CONSP, the list itself.  We follow the CONSP test
with XCAR, which crashed.  The only way I could imagine that happening
is that the list got modified between the test and the XCAR call.  Is
there any other way to explain that?

>     How else would you explain a segfault in that loop?
> 
> It's not clear the code is in that loop.

The address clearly points to Fplist_member at fns.c:2879.

> It could merely be RtlCaptureStackBackTrace misbehaving, no?

I don't think so.  It could be something else, though: the segfault
could have happened in another thread while the Lisp thread was in
plist-member.

But the change I made cannot do any harm, and it tried to use the only
evidence I had from the report.  The alternative was to do nothing at
all.

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: emacs-devel@gnu.org
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Mon, 12 Oct 2015 20:36:06 -0700
>> 
>>     QUIT could call some Lisp.
>>
>> Sure, but Lisp cannot affect the value of CONSP (plist).
>
> Not the value of CONSP, the list itself.  We follow the CONSP test
> with XCAR, which crashed.  The only way I could imagine that happening
> is that the list got modified between the test and the XCAR call.  Is
> there any other way to explain that?

Huh?  CONSP only checks some bits on a pointer.  It does not verify that
the pointer can be dereferenced into an existing cons cell.  So there is
absolutely no need to modify anything between having a true CONSP test
and a crashing XCAR.

-- 
David Kastrup

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Eli Zaretskii]

> From: David Kastrup <dak@gnu.org>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Tue, 13 Oct 2015 17:08:52 +0200
> 
> CONSP only checks some bits on a pointer.  It does not verify that
> the pointer can be dereferenced into an existing cons cell.  So there is
> absolutely no need to modify anything between having a true CONSP test
> and a crashing XCAR.

Yes, well...  I didn't have such a serious calamity in mind.

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: David Kastrup <dak@gnu.org>
>> Cc: Paul Eggert <eggert@cs.ucla.edu>, monnier@iro.umontreal.ca,
>> emacs-devel@gnu.org
>> Date: Tue, 13 Oct 2015 17:08:52 +0200
>> 
>> CONSP only checks some bits on a pointer.  It does not verify that
>> the pointer can be dereferenced into an existing cons cell.  So there is
>> absolutely no need to modify anything between having a true CONSP test
>> and a crashing XCAR.
>
> Yes, well...  I didn't have such a serious calamity in mind.

Shrug.  An accidental collection when the value is currently in a local
variable clearly having to end up in the stack frame seems less likely
to me than an accidental collection at a time when the value was not in
active use but likely only stored in some heap data structure.  Who
marks the structures in those?

If the calamity is indeed premature collection rather than a random
value that was never valid to start with.

-- 
David Kastrup

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Paul Eggert]

Eli Zaretskii wrote:

> The only way I could imagine that happening
> is that the list got modified between the test and the XCAR call.

As David mentioned, no modification to the list can explain a crashed XCAR if a 
valid CONSP succeeded.


>> >It's not clear the code is in that loop.
> The address clearly points to Fplist_member at fns.c:2879.

Yes, but that part of the backtrace looks wrong.  Why would Fplist_member be 
calling itself recursively?  Most likely the backtrace is bogus and the patch 
irrelevant.

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 13 Oct 2015 08:21:27 -0700
> 
> >> >It's not clear the code is in that loop.
> > The address clearly points to Fplist_member at fns.c:2879.
> 
> Yes, but that part of the backtrace looks wrong.  Why would Fplist_member be 
> calling itself recursively?

No, the backtrace doesn't say that.  There are 2 separate backtraces
lumped into a single file, that's all.

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Stefan Monnier]

> Not the value of CONSP, the list itself.  We follow the CONSP test
> with XCAR, which crashed.  The only way I could imagine that happening
> is that the list got modified between the test and the XCAR call.  Is
> there any other way to explain that?

The result of the CONSP test shouldn't be affected by anything at all
(it's basically testing some bit-pattern on the integer value stored in
the local variable, so no external code should be able to change it
since we don't pass any reference to that variable anywhere).

So doing moving the CONSP test before/after the QUIT shouldn't make any
difference.  Of course, if the Lisp_Object value is a reference to
a cons cell that got GC'd away, CONSP will still return true while XCAR
could crash or return garbage.  Similarly, if the Lisp_Object value is
garbage, CONSP might return true while XCAR might crash (or return
further garbage).

I think the symptoms you describe seem to indicate that we have
a problem where the GC collected a cons cell that was still in use
(otherwise, if CONSP is true, than XCAR should not crash, no matter what
happens between the two).


        Stefan

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Eli Zaretskii <eliz@gnu.org>
>> Date: Mon, 12 Oct 2015 21:07:00 -0400
>> 
>> >     Attempt to avoid crashes in plist-member
>>     
>> >     * src/fns.c (Fplist_member): Don't call QUIT between a CONSP test
>> >     and a call to XCDR.  (Bug#21655)
>> 
>> I have no objection to the patch itself, but I must say that I wonder
>> how it can avoid crashes.  IOW why would calling QUIT between a CONSP test
>> and a call to XCDR be dangerous?
>
> QUIT could call some Lisp.
>
> How else would you explain a segfault in that loop?

The only chance I see if garbage collection mistakenly collects the
referenced cell.  But that wouldn't be cured by the change.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
> Date: Tue, 13 Oct 2015 10:00:26 +0200
> 
> > QUIT could call some Lisp.
> >
> > How else would you explain a segfault in that loop?
> 
> The only chance I see if garbage collection mistakenly collects the
> referenced cell.  But that wouldn't be cured by the change.

Couldn't some atimer call some (possibly advised) Lisp, which could
change the list that is being iterated through?

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Eli Zaretskii <eliz@gnu.org>
>> Date: Mon, 12 Oct 2015 21:07:00 -0400
>> 
>> >     Attempt to avoid crashes in plist-member
>>     
>> >     * src/fns.c (Fplist_member): Don't call QUIT between a CONSP test
>> >     and a call to XCDR.  (Bug#21655)
>> 
>> I have no objection to the patch itself, but I must say that I wonder
>> how it can avoid crashes.  IOW why would calling QUIT between a CONSP test
>> and a call to XCDR be dangerous?
>
> QUIT could call some Lisp.

Sure, but it would not return.  So the XCDR should not be an issue.

> How else would you explain a segfault in that loop?

Have you configured with

--no-thread-jumps

?

'-fthread-jumps'
     Perform optimizations that check to see if a jump branches to a
     location where another comparison subsumed by the first is found.
     If so, the first branch is redirected to either the destination of
     the second branch or a point immediately following it, depending on
     whether the condition is known to be true or false.

     Enabled at levels '-O2', '-O3', '-Os'.

Because when jumps/calls are threaded, most particularly to functions
the compiler knows not to return, the backtraces may point to unrelated
code and with nonsensical local variable settings.

-- 
David Kastrup

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Stefan Monnier]

>> QUIT could call some Lisp.
> Sure, but it would not return.

Actually, QUIT can run Elisp and return (the `quit' signal is the only
signal that can do that).


        Stefan

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> QUIT could call some Lisp.
>> Sure, but it would not return.
>
> Actually, QUIT can run Elisp and return (the `quit' signal is the only
> signal that can do that).

I have my doubt this could be the cause of the problem: between CONSP
and XCDR the value has to be kept somewhere: in a register or on the
stack.  If Lisp code were running garbage collection in between, this
cons cell would be protected due to its presence in the stack frame.

The only plausible explanation I'd have is that CONSP happens to be true
(due to the set bits in the value) but the value is garbage anyway and
points nowhere sane.  In that case, any intermittent QUIT call would be
a red herring: the XCDR would bork on the value anyway.

-- 
David Kastrup

Re: [Emacs-diffs] master 8ba156f: Attempt to avoid crashes in plist-member

[Eli Zaretskii]

> From: David Kastrup <dak@gnu.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
> Date: Tue, 13 Oct 2015 12:26:11 +0200
> 
> > QUIT could call some Lisp.
> 
> Sure, but it would not return.  So the XCDR should not be an issue.

I meant via atimers.  Those do return.

> Have you configured with
> 
> --no-thread-jumps
> 
> ?
> 
> '-fthread-jumps'
>      Perform optimizations that check to see if a jump branches to a
>      location where another comparison subsumed by the first is found.
>      If so, the first branch is redirected to either the destination of
>      the second branch or a point immediately following it, depending on
>      whether the condition is known to be true or false.
> 
>      Enabled at levels '-O2', '-O3', '-Os'.

I think that build is not optimized at all.