bug#6378: all-completions Segfault

bug#6378: all-completions Segfault

[Nathan Weizenbaum]

[-- Attachment #1: Type: text/plain, Size: 239 bytes --]

This segfaults me on GNU Emacs 24.0.50.1 (i686-pc-linux-gnu, GTK+ Version
2.16.1) of 2010-05-09:

  (all-completions "" [])

I think the problem is on line 1593 of src/minibuf.c, but my
Emacs-innards-fu isn't good enough to attempt a fix.

[-- Attachment #2: Type: text/html, Size: 270 bytes --]

bug#6378: all-completions Segfault

[Lawrence Mitchell]

Nathan Weizenbaum wrote:
> This segfaults me on GNU Emacs 24.0.50.1 (i686-pc-linux-gnu, GTK+ Version
> 2.16.1) of 2010-05-09:

>   (all-completions "" [])

> I think the problem is on line 1593 of src/minibuf.c, but my
> Emacs-innards-fu isn't good enough to attempt a fix.

I think this patch should fix things

diff --git a/src/minibuf.c b/src/minibuf.c
index ad81bfd..1d93901 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -1590,7 +1590,7 @@ with a space are ignored unless STRING itself starts with a space.  */)
   if (type == 2)
     {
       obsize = XVECTOR (collection)->size;
-      bucket = XVECTOR (collection)->contents[index];
+      bucket = obsize == 0 ? zero : XVECTOR (collection)->contents[index];
     }
 
   while (1)


Although I don't understand why the code-path for the vector
version can't be simplified as in the following patch, which also
fixes the problem AFAICT:

diff --git a/src/minibuf.c b/src/minibuf.c
index ad81bfd..c6aae27 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -1610,22 +1610,14 @@ with a space are ignored unless STRING itself starts with a space.  */)
 	}
       else if (type == 2)
 	{
-	  if (!EQ (bucket, zero))
-	    {
-	      elt = bucket;
-	      eltstring = elt;
-	      if (XSYMBOL (bucket)->next)
-		XSETSYMBOL (bucket, XSYMBOL (bucket)->next);
-	      else
-		XSETFASTINT (bucket, 0);
-	    }
-	  else if (++index >= obsize)
-	    break;
-	  else
-	    {
-	      bucket = XVECTOR (collection)->contents[index];
-	      continue;
-	    }
+          if ( index < obsize )
+            {
+              elt = bucket;
+              eltstring = elt;
+              bucket = XVECTOR (collection)->contents[++index];
+            }
+          else
+            break;
 	}
       else /* if (type == 3) */
 	{

Cheers,
Lawrence
-- 
Lawrence Mitchell <wence@gmx.li>

bug#6378: all-completions Segfault

[Juanma Barranquero]

On Tue, Jun 8, 2010 at 17:46, Nathan Weizenbaum <nex342@gmail.com> wrote:

>   (all-completions "" [])
>
> I think the problem is on line 1593 of src/minibuf.c, but my
> Emacs-innards-fu isn't good enough to attempt a fix.

minibuf.c:1617, at this code:

	      if (XSYMBOL (bucket)->next)

because bucket has been assigned random junk from the nonexistent item
at position 0 in COLLECTION.

It should be fixed with the attached patch.

    Juanma


2010-06-08  Juanma Barranquero  <lekktu@gmail.com>

	* minibuf.c (Fall_completions): Check COLLECTION's size.  (Bug#6378)


=== modified file 'src/minibuf.c'
--- src/minibuf.c	2010-01-13 08:35:10 +0000
+++ src/minibuf.c	2010-06-08 16:34:41 +0000
@@ -1591,5 +1591,5 @@
     {
       obsize = XVECTOR (collection)->size;
-      bucket = XVECTOR (collection)->contents[index];
+      bucket = obsize ? XVECTOR (collection)->contents[index] : zero;
     }

bug#6378: all-completions Segfault

[Juanma Barranquero]

I've installed the fix on the emacs-23 branch.

    Juanma

bug#6378: all-completions Segfault

[Lennart Borgman]

On Tue, Jun 8, 2010 at 6:53 PM, Juanma Barranquero <lekktu@gmail.com> wrote:
> I've installed the fix on the emacs-23 branch.

Isn't there a similar case in Ftry_completion?

bug#6378: all-completions Segfault

[Juanma Barranquero]

On Tue, Jun 8, 2010 at 20:58, Lennart Borgman <lennart.borgman@gmail.com> wrote:

> Isn't there a similar case in Ftry_completion?

Apparently no.

ELISP> (try-completion "" [])
*** Eval error ***  Wrong type argument: vectorp, []
ELISP> (try-completion "" [[]])
*** Eval error ***  Bad data in guts of obarray
ELISP> (try-completion "" [0])
nil
ELISP> (try-completion "" [a])
""
ELISP>

    Juanma

bug#6378: all-completions Segfault

[Lennart Borgman]

On Tue, Jun 8, 2010 at 9:08 PM, Juanma Barranquero <lekktu@gmail.com> wrote:
> On Tue, Jun 8, 2010 at 20:58, Lennart Borgman <lennart.borgman@gmail.com> wrote:
>
>> Isn't there a similar case in Ftry_completion?
>
> Apparently no.
>
> ELISP> (try-completion "" [])
> *** Eval error ***  Wrong type argument: vectorp, []
> ELISP> (try-completion "" [[]])
> *** Eval error ***  Bad data in guts of obarray
> ELISP> (try-completion "" [0])
> nil
> ELISP> (try-completion "" [a])
> ""
> ELISP>
>
>    Juanma


So the thing protecting it is that an obarray can't have length 0? Can
one be sure that does not break someday?

bug#6378: all-completions Segfault

[Juanma Barranquero]

On Tue, Jun 8, 2010 at 21:32, Lennart Borgman <lennart.borgman@gmail.com> wrote:

> So the thing protecting it is that an obarray can't have length 0? Can
> one be sure that does not break someday?

Meaning?

    Juanma

bug#6378: all-completions Segfault

[Stefan Monnier]

> Although I don't understand why the code-path for the vector
> version can't be simplified as in the following patch, which also
> fixes the problem AFAICT:

> diff --git a/src/minibuf.c b/src/minibuf.c
> index ad81bfd..c6aae27 100644
> --- a/src/minibuf.c
> +++ b/src/minibuf.c
> @@ -1610,22 +1610,14 @@ with a space are ignored unless STRING itself starts with a space.  */)
>  	}
>        else if (type == 2)
>  	{
> -	  if (!EQ (bucket, zero))
> -	    {
> -	      elt = bucket;
> -	      eltstring = elt;
> -	      if (XSYMBOL (bucket)->next)
> -		XSETSYMBOL (bucket, XSYMBOL (bucket)->next);
> -	      else
> -		XSETFASTINT (bucket, 0);
> -	    }
> -	  else if (++index >= obsize)
> -	    break;
> -	  else
> -	    {
> -	      bucket = XVECTOR (collection)->contents[index];
> -	      continue;
> -	    }
> +          if ( index < obsize )
> +            {
> +              elt = bucket;
> +              eltstring = elt;
> +              bucket = XVECTOR (collection)->contents[++index];
> +            }
> +          else
> +            break;
>  	}
>        else /* if (type == 3) */
>  	{

IIUC this would only loop through all the buckets, without looping
through each bucket's linked list.
Compare (length obarray)
and     (let ((i 0)) (mapatoms (lambda (_) (incf i)) obarray) i)


        Stefan

bug#6378: all-completions Segfault

[Thierry Volpiatto]

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

>> Although I don't understand why the code-path for the vector
>> version can't be simplified as in the following patch, which also
>> fixes the problem AFAICT:
>
>> diff --git a/src/minibuf.c b/src/minibuf.c
>> index ad81bfd..c6aae27 100644
>> --- a/src/minibuf.c
>> +++ b/src/minibuf.c
>> @@ -1610,22 +1610,14 @@ with a space are ignored unless STRING itself starts with a space.  */)
>>  	}
>>        else if (type == 2)
>>  	{
>> -	  if (!EQ (bucket, zero))
>> -	    {
>> -	      elt = bucket;
>> -	      eltstring = elt;
>> -	      if (XSYMBOL (bucket)->next)
>> -		XSETSYMBOL (bucket, XSYMBOL (bucket)->next);
>> -	      else
>> -		XSETFASTINT (bucket, 0);
>> -	    }
>> -	  else if (++index >= obsize)
>> -	    break;
>> -	  else
>> -	    {
>> -	      bucket = XVECTOR (collection)->contents[index];
>> -	      continue;
>> -	    }
>> +          if ( index < obsize )
>> +            {
>> +              elt = bucket;
>> +              eltstring = elt;
>> +              bucket = XVECTOR (collection)->contents[++index];
>> +            }
>> +          else
>> +            break;
>>  	}
>>        else /* if (type == 3) */
>>  	{
>
> IIUC this would only loop through all the buckets, without looping
> through each bucket's linked list.
> Compare (length obarray)
> and     (let ((i 0)) (mapatoms (lambda (_) (incf i)) obarray) i)

Don't know if that related but

(completing-read "test: " [1 2 3 23 24 34 26 40 28])

test: 2 ==> TAB

instead of failing crash emacs.

 -- 
Thierry Volpiatto
Gpg key: http://pgp.mit.edu/

bug#6378: all-completions Segfault

[Lennart Borgman]

On Wed, Jun 9, 2010 at 7:13 AM, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:
> (completing-read "test: " [1 2 3 23 24 34 26 40 28])

Does not that crash just because only symbols or strings can be keys
in the collection?

bug#6378: all-completions Segfault

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
>>> Although I don't understand why the code-path for the vector
>>> version can't be simplified as in the following patch, which also
>>> fixes the problem AFAICT:
>>
>>> diff --git a/src/minibuf.c b/src/minibuf.c
>>> index ad81bfd..c6aae27 100644
>>> --- a/src/minibuf.c
>>> +++ b/src/minibuf.c
>>> @@ -1610,22 +1610,14 @@ with a space are ignored unless STRING itself starts with a space.  */)
>>>  	}
>>>        else if (type == 2)
>>>  	{
>>> -	  if (!EQ (bucket, zero))
>>> -	    {
>>> -	      elt = bucket;
>>> -	      eltstring = elt;
>>> -	      if (XSYMBOL (bucket)->next)
>>> -		XSETSYMBOL (bucket, XSYMBOL (bucket)->next);
>>> -	      else
>>> -		XSETFASTINT (bucket, 0);
>>> -	    }
>>> -	  else if (++index >= obsize)
>>> -	    break;
>>> -	  else
>>> -	    {
>>> -	      bucket = XVECTOR (collection)->contents[index];
>>> -	      continue;
>>> -	    }
>>> +          if ( index < obsize )
>>> +            {
>>> +              elt = bucket;
>>> +              eltstring = elt;
>>> +              bucket = XVECTOR (collection)->contents[++index];
>>> +            }
>>> +          else
>>> +            break;
>>>  	}
>>>        else /* if (type == 3) */
>>>  	{
>>
>> IIUC this would only loop through all the buckets, without looping
>> through each bucket's linked list.
>> Compare (length obarray)
>> and     (let ((i 0)) (mapatoms (lambda (_) (incf i)) obarray) i)
>
> Don't know if that related but
>
> (completing-read "test: " [1 2 3 23 24 34 26 40 28])
>
> test: 2 ==> TAB
>
> instead of failing crash emacs.

Program received signal SIGSEGV, Segmentation fault.
0x08151015 in Fall_completions ()

-- 
Thierry Volpiatto
Gpg key: http://pgp.mit.edu/

bug#6378: all-completions Segfault

[Thierry Volpiatto]

Lennart Borgman <lennart.borgman@gmail.com> writes:

> On Wed, Jun 9, 2010 at 7:13 AM, Thierry Volpiatto
> <thierry.volpiatto@gmail.com> wrote:
>> (completing-read "test: " [1 2 3 23 24 34 26 40 28])
>
> Does not that crash just because only symbols or strings can be keys
> in the collection?
I thought but it's worst, because:

(completing-read "test: " ["1" "2" "23" "24" "25" "34" "45" "56"])

Also crash emacs.

-- 
Thierry Volpiatto
Gpg key: http://pgp.mit.edu/

bug#6378: all-completions Segfault

[Lennart Borgman]

On Wed, Jun 9, 2010 at 9:23 AM, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:
> Lennart Borgman <lennart.borgman@gmail.com> writes:
>
>> On Wed, Jun 9, 2010 at 7:13 AM, Thierry Volpiatto
>> <thierry.volpiatto@gmail.com> wrote:
>>> (completing-read "test: " [1 2 3 23 24 34 26 40 28])
>>
>> Does not that crash just because only symbols or strings can be keys
>> in the collection?
> I thought but it's worst, because:
>
> (completing-read "test: " ["1" "2" "23" "24" "25" "34" "45" "56"])
>
> Also crash emacs.

Yes, it expects an obarray, not a plain vector.

bug#6378: all-completions Segfault

[Lawrence Mitchell]

Stefan Monnier wrote:
[...]

> IIUC this would only loop through all the buckets, without looping
> through each bucket's linked list.
> Compare (length obarray)
> and     (let ((i 0)) (mapatoms (lambda (_) (incf i)) obarray) i)

That makes sense, thanks.

Lawrence

-- 
Lawrence Mitchell <wence@gmx.li>

bug#6378: all-completions Segfault

[Thierry Volpiatto]

Lennart Borgman <lennart.borgman@gmail.com> writes:

> On Wed, Jun 9, 2010 at 9:23 AM, Thierry Volpiatto
> <thierry.volpiatto@gmail.com> wrote:
>> Lennart Borgman <lennart.borgman@gmail.com> writes:
>>
>>> On Wed, Jun 9, 2010 at 7:13 AM, Thierry Volpiatto
>>> <thierry.volpiatto@gmail.com> wrote:
>>>> (completing-read "test: " [1 2 3 23 24 34 26 40 28])
>>>
>>> Does not that crash just because only symbols or strings can be keys
>>> in the collection?
>> I thought but it's worst, because:
>>
>> (completing-read "test: " ["1" "2" "23" "24" "25" "34" "45" "56"])
>>
>> Also crash emacs.
>
> Yes, it expects an obarray, not a plain vector.

Anyway if completing-read is not able to handle a vector 
it should return an error and not crashing emacs.

-- 
Thierry Volpiatto
Gpg key: http://pgp.mit.edu/

bug#6378: all-completions Segfault

[Andreas Schwab]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Anyway if completing-read is not able to handle a vector 
> it should return an error and not crashing emacs.

This is already fixed in emacs-23.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#6378: all-completions Segfault

[Thierry Volpiatto]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Anyway if completing-read is not able to handle a vector 
>> it should return an error and not crashing emacs.
>
> This is already fixed in emacs-23.
I don't understand, it is fixed in emacs-23 and not in emacs-24?

I am speaking of:
GNU Emacs 24.0.50.1 (i686-pc-linux-gnu, GTK+ Version 2.18.9)
 of 2010-06-08 on tux

-- 
Thierry Volpiatto
Gpg key: http://pgp.mit.edu/

bug#6378: all-completions Segfault

[Andreas Schwab]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> I don't understand, it is fixed in emacs-23 and not in emacs-24?

Because nobody merged it yet.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."