bug#13074: VM Segfaults with Bad `Call' Instruction

bug#13074: VM Segfaults with Bad `Call' Instruction

[Noah Lavine]

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

Hello,

This is an interesting bug, because the only way to hit it (as far as I can
tell) is to mess up when writing a compiler. However, I did mess up, and I
discover that I can generate a `call' instruction in the trunk VM where the
procedure to call will be 0x0. Then the VM will try to check whether the
procedure is really a procedure, and Guile will segfault at line 796 of
v-i-system.c.

I think the correct behavior would be to throw a `vm-bad-instruction' error
instead. The fix should be pretty simple - just check if program is 0x0 and
jump to vm-bad-instruction in that case.

Noah

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

bug#13074: VM Segfaults with Bad `Call' Instruction

[Noah Lavine]

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

The following patch fixes the problem for me:

diff --git a/libguile/vm-i-system.c b/libguile/vm-i-system.c
index 7153ab5..dff2ab2 100644
--- a/libguile/vm-i-system.c
+++ b/libguile/vm-i-system.c
@@ -793,7 +793,9 @@ VM_DEFINE_INSTRUCTION (55, call, "call", 1, -1, 1)

   VM_HANDLE_INTERRUPTS;

-  if (SCM_UNLIKELY (!SCM_PROGRAM_P (program)))
+  if (SCM_UNLIKELY (program == NULL))
+    goto vm_error_bad_instruction;
+  else if (SCM_UNLIKELY (!SCM_PROGRAM_P (program)))
     {
       if (SCM_STRUCTP (program) && SCM_STRUCT_APPLICABLE_P (program))
         {

Any objections if I apply it to stable-2.0? (Or master?)

Noah


On Mon, Dec 3, 2012 at 10:06 PM, Noah Lavine <noah.b.lavine@gmail.com>wrote:

> Hello,
>
> This is an interesting bug, because the only way to hit it (as far as I
> can tell) is to mess up when writing a compiler. However, I did mess up,
> and I discover that I can generate a `call' instruction in the trunk VM
> where the procedure to call will be 0x0. Then the VM will try to check
> whether the procedure is really a procedure, and Guile will segfault at
> line 796 of v-i-system.c.
>
> I think the correct behavior would be to throw a `vm-bad-instruction'
> error instead. The fix should be pretty simple - just check if program is
> 0x0 and jump to vm-bad-instruction in that case.
>
> Noah
>
>

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

bug#13074: VM Segfaults with Bad `Call' Instruction

[Ludovic Courtès]

Hi Noah,

Noah Lavine <noah.b.lavine@gmail.com> skribis:

> diff --git a/libguile/vm-i-system.c b/libguile/vm-i-system.c
> index 7153ab5..dff2ab2 100644
> --- a/libguile/vm-i-system.c
> +++ b/libguile/vm-i-system.c
> @@ -793,7 +793,9 @@ VM_DEFINE_INSTRUCTION (55, call, "call", 1, -1, 1)
>
>    VM_HANDLE_INTERRUPTS;
>
> -  if (SCM_UNLIKELY (!SCM_PROGRAM_P (program)))
> +  if (SCM_UNLIKELY (program == NULL))
> +    goto vm_error_bad_instruction;
> +  else if (SCM_UNLIKELY (!SCM_PROGRAM_P (program)))
>      {
>        if (SCM_STRUCTP (program) && SCM_STRUCT_APPLICABLE_P (program))
>          {

I’d rather not apply it, because it adds overhead for every call for a
situation that cannot happen when using Guile’s compiler, IIUC.

WDYT?

Thanks,
Ludo’.

bug#13074: VM Segfaults with Bad `Call' Instruction

[Noah Lavine]

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

That makes sense. I hit this error in debugging a CPS->GLIL compiler (which
I hope will become Guile's compiler, but that's another story). However,
once the debugging is done, I suppose it won't make a difference.

What do you think about enabling it only in the debug VM, or something like
that? Then if there's some way for me to run my code in debug mode, I can
get the better output without slowing down most things.

Noah


On Wed, Dec 5, 2012 at 9:10 AM, Ludovic Courtès <ludo@gnu.org> wrote:

> Hi Noah,
>
> Noah Lavine <noah.b.lavine@gmail.com> skribis:
>
> > diff --git a/libguile/vm-i-system.c b/libguile/vm-i-system.c
> > index 7153ab5..dff2ab2 100644
> > --- a/libguile/vm-i-system.c
> > +++ b/libguile/vm-i-system.c
> > @@ -793,7 +793,9 @@ VM_DEFINE_INSTRUCTION (55, call, "call", 1, -1, 1)
> >
> >    VM_HANDLE_INTERRUPTS;
> >
> > -  if (SCM_UNLIKELY (!SCM_PROGRAM_P (program)))
> > +  if (SCM_UNLIKELY (program == NULL))
> > +    goto vm_error_bad_instruction;
> > +  else if (SCM_UNLIKELY (!SCM_PROGRAM_P (program)))
> >      {
> >        if (SCM_STRUCTP (program) && SCM_STRUCT_APPLICABLE_P (program))
> >          {
>
> I’d rather not apply it, because it adds overhead for every call for a
> situation that cannot happen when using Guile’s compiler, IIUC.
>
> WDYT?
>
> Thanks,
> Ludo’.
>

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

bug#13074: VM Segfaults with Bad `Call' Instruction

[Ludovic Courtès]

Noah Lavine <noah.b.lavine@gmail.com> skribis:

> That makes sense. I hit this error in debugging a CPS->GLIL compiler (which
> I hope will become Guile's compiler, but that's another story). However,
> once the debugging is done, I suppose it won't make a difference.

Oooh, make sure to post about your plans.

> What do you think about enabling it only in the debug VM, or something like
> that? Then if there's some way for me to run my code in debug mode, I can
> get the better output without slowing down most things.

I’m inclined to leave it as is, because it’s only hit when generating
wrong code.  How strongly do you feel about it?  :-)

Ludo’.

bug#13074: VM Segfaults with Bad `Call' Instruction

[Noah Lavine]

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

On Wed, Dec 5, 2012 at 5:14 PM, Ludovic Courtès <ludo@gnu.org> wrote:

> Noah Lavine <noah.b.lavine@gmail.com> skribis:
>
> > That makes sense. I hit this error in debugging a CPS->GLIL compiler
> (which
> > I hope will become Guile's compiler, but that's another story). However,
> > once the debugging is done, I suppose it won't make a difference.
>
> Oooh, make sure to post about your plans.


I will post more when I have more code to show, but basically it's the same
idea as the CPS-to-RTL experiment earlier. The difference is that in that
post I said that adding two new things at the same time (CPS and RTL) was
probably a bad idea. Now I'm doing something about it, by making the CPS
compiler generate GLIL instead. I hope this will be an easier path towards
a nicer compiler.


> > What do you think about enabling it only in the debug VM, or something
> like
> > that? Then if there's some way for me to run my code in debug mode, I can
> > get the better output without slowing down most things.
>
> I’m inclined to leave it as is, because it’s only hit when generating
> wrong code.  How strongly do you feel about it?  :-)
>

Well, I just fixed the bug, so I feel fine right now. :-)

In general, I do think there should at least be an option for having full
error-checking in the VM. It would have been much, much harder for me to
find this without having patched the VM, because it would have taken me a
very long time to try each new thing I tried, because I would have had to
restart Guile. I am happy for it not to be on the regular code-path,
though. I also realize that writing a compiler is an unusual application,
so maybe it should even be a compile-time option for users who prefer their
Guile slow. How does that sound?

Noah

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

bug#13074: VM Segfaults with Bad `Call' Instruction

[Ludovic Courtès]

Hi!

Noah Lavine <noah.b.lavine@gmail.com> skribis:

> In general, I do think there should at least be an option for having full
> error-checking in the VM. It would have been much, much harder for me to
> find this without having patched the VM, because it would have taken me a
> very long time to try each new thing I tried, because I would have had to
> restart Guile. I am happy for it not to be on the regular code-path,
> though. I also realize that writing a compiler is an unusual application,
> so maybe it should even be a compile-time option for users who prefer their
> Guile slow. How does that sound?

The VM does full error checking.  But there’s a difference between
checking whether an object has the expected type, and checking whether
an object is a well-formed ‘SCM’ object (and NULL is not a valid ‘SCM’
object.)

Guile never does the latter, and as a rule of thumb I would keep things
this way.

The brave hacker working on a compiler can easily figure out what how to
debug all sorts of crazy things.  :-)

So I’m closing it for now.

Thanks,
Ludo’.

PS: It’s still unclear to me how you ended up forging an invalid SCM
    object.  I think you either have to generate invalid bytecode, or to
    use (pointer->scm %null-pointer), or variants thereof.

bug#13074: VM Segfaults with Bad `Call' Instruction

[Noah Lavine]

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

On Tue, Dec 11, 2012 at 4:42 AM, Ludovic Courtès <ludo@gnu.org> wrote:
>
> The VM does full error checking.  But there’s a difference between
> checking whether an object has the expected type, and checking whether
> an object is a well-formed ‘SCM’ object (and NULL is not a valid ‘SCM’
> object.)
>
> Guile never does the latter, and as a rule of thumb I would keep things
> this way.
>

Okay.


> The brave hacker working on a compiler can easily figure out what how to
> debug all sorts of crazy things.  :-)
>

Yes, "easily". :-)


> So I’m closing it for now.
>
> Thanks,
> Ludo’.
>
> PS: It’s still unclear to me how you ended up forging an invalid SCM
>     object.  I think you either have to generate invalid bytecode, or to
>     use (pointer->scm %null-pointer), or variants thereof.
>

I loaded a procedure on the stack, used the new-frame instruction, and then
the call instruction. When I switched the order of the first two things,
the problem went away. I must have been using uninitialized stack space.

Noah

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