On Sat, 29 Sep 2012, Jon Trulson wrote:

> On Sun, 30 Sep 2012, Marcin Cieslak wrote:

> Looks like menory corruption bug somewhere - somethingis scribbling
> over this memory...

No, va_args promotes chars and shorts to ints, but it does not
automatically promote ints to longs.

> > Providing a long value on None (0L) as in this change fixes the
> > problem.
> > 
> > I understand is that it possible to use such an "empty" widget
> > is to create additional space at the bottom of the newly created
> > "icon_scrolled_win".
> > 
> > What needs to be clarified - shouldn't be such an (int) value be
> > automatically promoted to (long) (or XtArgVal, XtPointer, ...)
> 
> Yes, there is not problem I am aware of with int 0 being promoted to
> long 0.  I've never heard of a problem where ints are erroneously
> promoted to longs.  The other way around of course is problematic,
> but... there is something else wrong here.

The problem is, they are *not* promoted (fine according to C,
a bit troublesome whem coming to the X Toolkit).

TL;DR: I have reasons to believe that my fix is correct :)

Here's what happens:

XtVaCreateManagedWidget invocation looks like this before patching:

Let's create the assembler file in cde/programs/dtcreate with the command:

gcc -c -S -g -pipe  -I.  -I../.. -I../../exports/include 
-I../../imports/motif/include -I../../imports/x11/include -I/usr/local/include 
-DCSRG_BASED -DANSICPP -DMULTIBYTE -DNLS16      -DOSMAJORVERSION=10 
-DOSMINORVERSION=0      icon_selection_dialog.c

icon_selection_dialog.s:

  3983          .loc 1 1768 0
                /* ... */
  4004          movq    UxIcon_selection_dialogContext(%rip), %rax
  4005          movq    (%rax), %rax
  4006          movq    %rax, -1128(%rbp)
  4007          movq    xmScrolledWindowWidgetClass(%rip), %rax
  4008          movq    %rax, -424(%rbp)
  4009          movq    $0, 216(%rsp)    <-- final NULL, 64-bit 0
  4010          movl    $3, 208(%rsp)    <-- XmATTACH_WIDGET, 32-bit 3
  4011          movq    %rdx, 200(%rsp)
  4012          movl    $0, 192(%rsp)    <-- 32-bit 0 for XmNbottomWidget
  4013          movq    %rcx, 184(%rsp)
  4014          movl    $10, 176(%rsp)   <-- 32-bit 10 for XmNbottomOffset
  4015          movq    %rsi, 168(%rsp)
  4016          movl    $4, 160(%rsp)    <-- XmATTACH_OPPOSITE_WIDGET, 32-bit 4
  4017          movq    %rdi, 152(%rsp)
  4018          movq    %r8, 144(%rsp)     
  4019          movq    %r9, 136(%rsp)
  4020          movl    $0, 128(%rsp)
  4021          movq    %r10, 120(%rsp)
  4022          movl    $3, 112(%rsp)
  4023          movq    %r11, 104(%rsp)
  4024          movq    %r12, 96(%rsp)
  4025          movq    %r13, 88(%rsp)
  4026          movl    $0, 80(%rsp)
  4027          movq    %r14, 72(%rsp)
  4028          movl    $1, 64(%rsp)
  4029          movq    %r15, 56(%rsp)
  4030          movl    $10, 48(%rsp)
  4031          movq    -464(%rbp), %rdx
  4032          movq    %rdx, 40(%rsp)
  4033          movl    $1, 32(%rsp)
  4034          movq    -456(%rbp), %rcx
  4035          movq    %rcx, 24(%rsp)
  4036          movl    $84, 16(%rsp)       <-- 84 32-bit for XmNy
  4037          movq    -448(%rbp), %rax
  4038          movq    %rax, 8(%rsp)       
  4039          movl    $282, (%rsp)        <-- 282 32-bit for XmNx
  4040          movq    -440(%rbp), %r9
  4041          movl    $0, %r8d
  4042          movq    -432(%rbp), %rcx
  4043          movq    -1128(%rbp), %rdx
  4044          movq    -424(%rbp), %rsi
  4045          movl    $.LC49, %edi
  4046          movl    $0, %eax
  4047          call    XtVaCreateManagedWidget

In the above listing we can see that plain integer 
values (int, enum, etc.) are stored on stack using
32-bit movl. Long, pointer values are stored on
stack using 64-bit movq. As stack values are aligned
on 8 bytes anyway, this means that upper 32 bits
are left as they were - that means they are undefined.

My patch changes only this:
 
  4012         movl    $0, 192(%rsp)      <-- 32-bit 0 for XmNbottomWidget

to this:

  4012         movq    $0, 192(%rsp)

Because XmATTACH_NONE - 32 bit integer zero becomes long None 
- 64 bit integer zero and a whole 64 bit word is cleared.

Further we need to examine _XtVaToTypedArgList from X Toolkit
that fetches variadic arguments from stack. This is the fragment
that deals with normal (name, value) pairs:

Varargs.c of libXt-1.1.1:
    505         } else {
    506             args[count].name = attr;
    507             args[count].type = NULL;
    508             args[count].value = va_arg(var, XtArgVal);
    509             ++count;
    510         }

This means that every value is taken from the stack
and coverted to XtArgVal value, which means a whole 64 bit 
value is taken in our case.

We can reproduce this if we have libXt sources
and debug symbols somewhere around.

For this we can use a small gdb script:

---8<--- .gdbrc begin ---
break icon_selection_dialog.c:1768
run
break Varargs.c:507
cont
display attr
display args[count].value
---8<--- .gdbrc end ---

then we start dtcreate:

gdb -x .gdbrc ./dtcreate

and then we just click "Find Set..."

it should stop:

Breakpoint 1 at 0x42112b: file icon_selection_dialog.c, line 1768.

Breakpoint 1, _Uxbuild_icon_selection_dialog () at icon_selection_dialog.c:1768
warning: Source file is more recent than executable.

1768            icon_scrolled_win = XtVaCreateManagedWidget( 
"icon_scrolled_win",
Breakpoint 2 at 0x801910814: file Varargs.c, line 507.

Breakpoint 2, _XtVaToTypedArgList (var=0x7fffffffbb40, max_count=15, 
    args_return=0x7fffffffbae8, num_args_return=0x7fffffffbae4)
    at Varargs.c:507
507                 args[count].type = NULL;
(gdb) n
508                 args[count].value = va_arg(var, XtArgVal);
2: args[count].value = -6510615555426900571
1: attr = 0x62c327 "scrollingPolicy"
(gdb) n
509                 ++count;
2: args[count].value = 0
1: attr = 0x62c327 "scrollingPolicy"

so there the value 0 is read for the scrollingPolicy parameter.

Let's contine and single step for the next parameter till
we reach line 509 again:

508                 args[count].value = va_arg(var, XtArgVal);
2: args[count].value = -6510615555426900571
1: attr = 0x630115 "x"
(gdb) n
509                 ++count;
2: args[count].value = 34359738650
1: attr = 0x630115 "x"

What is 34359738650? 

(gdb) print args[count].value & 0xffffffff 
$2 = 282

So, we have fetched a full 64-bit value from the stack
and only lower 32-bit were set by the "movl" instruction
by the caller.

This is *not* a problem because later this will be
converted to Dimension or some other 32-bit type
and upper bits will be truncated and the value "282"
will be used.

Let's type few times "cont" until we get to the "bottomOffset":

509                 ++count;
2: args[count].value = 10
1: attr = 0x62b20c "bottomOffset"
(gdb) cont
Continuing.

Breakpoint 2, _XtVaToTypedArgList (var=0x7fffffffbb40, max_count=15, 
    args_return=0x7fffffffbae8, num_args_return=0x7fffffffbae4)
    at Varargs.c:507
507                 args[count].type = NULL;
2: args[count].value = -6510615555426900571
1: attr = 0x62b24d "bottomWidget"
gdb) n
508                 args[count].value = va_arg(var, XtArgVal);
2: args[count].value = -6510615555426900571
1: attr = 0x62b24d "bottomWidget"
(gdb) n
509                 ++count;
2: args[count].value = 34359738368
1: attr = 0x62b24d "bottomWidget"

What is 34359738368? It's 0x800000000
- so the lower 32-bits are zero put there 
by the "movl" command - the same case as for the "x"
parameter above.

In this case it is a problem, because this
value is supposed to be a pointer to the Widget,
(NULL is allowed). So a full 64-bit value
will be passed to the code.

It crashes finally at Form.c:1955, in ConstraintInitialize()
(this is part of libXm):

Form.c:
  87 #define SIBLINGS(w,s)   (((w != NULL) && (s != NULL)) &&\
  88                          (XtParent(w) == XtParent(s)))

1952            if ((nc->att[i].type == XmATTACH_WIDGET) ||
1953                (nc->att[i].type == XmATTACH_OPPOSITE_WIDGET)) {
1954                
1955                while ((nc->att[i].w) && !SIBLINGS(nc->att[i].w, new_w)) {
1956                    nc->att[i].w = XtParent(nc->att[i].w);
1957                }
1958            }
(gdb) print nc->att[3]
$2 = {type = 3 '\003', w = 0x800000000, percent = 0, offset = 10, value = 0, 
  tempValue = 0}

Although this code and SIBLINGS macro are very careful about
not trying to dereference NULL pointer, the address
0x800000000 cannot be recognized as invalid. So it tries
to derefence this and we have segmentation fault.

Since there is nothing in va_arg() or Xt specification
that would tell us that "int value will be automically
promoted to long", I believe that compiler, libXt and libXm
are correct in this regard.

It is caller responsibility to set the argument correctly
as the null pointer to Widget (None from X.h can be used).

This is similar to attempts to use 0 for NULL pointers
as seen somewhere else in the code. And I must admint
dtcreate is poorly written, mosly in the pre-ANSI C.

This is bad news for us: apart from checking
parameters of all invocations of XtGetValues()
and XtVaGetValues() we need to check all invocations
of other XtVaXXX functions.

All variadic values where pointers, Widgets, etc. are expected
*must* be called with real 64-bit values without
any attempt to rely on non-existing conversion.

Probably it would be good to have a quick reference
of all Motif attibutes and their types to quickly
check only those that can cause problems.

//Marcin

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://ad.doubleclick.net/clk;258768047;13503038;j?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
cdesktopenv-devel mailing list
cdesktopenv-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cdesktopenv-devel

Reply via email to