#575 Macro branch operations may crash on 64-bit systems

release
closed-fixed
Program (402)
7
2007-03-04
2006-12-31
No

Building from Source on Linux with a 64-bit system leads to several error messages (casting from/to int from/to pointer.

After solving these compiler messages, the program runs. Using the preference file, it aborts with segfault when inserting something.
Without preferences (no Macros) it works.
Abort happens in ContinueMacro
at : status = (*inst)();
The 32-bit version with same preference file runs stable.

Discussion

  • Juergen Keidel
    Juergen Keidel
    2007-01-02

    Logged In: YES
    user_id=86514
    Originator: YES

    The reason for the segfault lies in interpreter.c, handling of branches.
    getting a value from PC results in compilerwarnings about different sizes of pointer and int. solving this by casting the void * of PC to long int get an address-like expansion of the value (i.e. extended to 0x7fffffff00000008 p.E)
    So the branching functions in interpreter.c must mask (or shift left32 and right back 32-bit of the value)
    This old trick of storing values in a pointer-location has problems at 64-Bit systems.

     
  • Scott Tringali
    Scott Tringali
    2007-01-02

    Logged In: YES
    user_id=11321
    Originator: NO

    Need a bit more detail on your 64-bit system. What CPU? Distro? Compiler? Version of NEdit?

    I don't dispute the code in there is wrong, it would help us to fix it if we can actually repro it. It doesn't repro for me on other 64-bit sytems I have access to.

    BTW: casting to long it will only put off the problem to another day for another platform.

     
  • Scott Tringali
    Scott Tringali
    2007-01-02

    • labels: 355501 --> Program
    • summary: build on 64-bit system --> Macro branch operations may crash on 64-bit systems
    • milestone: 103147 --> release
     
  • Scott Tringali
    Scott Tringali
    2007-01-02

    Fix, please review?

     
    Attachments
  • Scott Tringali
    Scott Tringali
    2007-01-02

    Logged In: YES
    user_id=11321
    Originator: NO

    The problem I see is that branching stuff in the parsing phase puts void* onto the stack, but pulls them off as integers (see AddImmediate vs. branchTrue).

    I changed them both to integers and made it more type-safe with a union, so the instructions won't take up any more space. There seems to be no reason for the cast to void* that I can find. The file now compiles with no warnings.

    I think the downside is that you can only branch an int's worth, instead of of large. A better solution would to make all the branches be of ptrdiff_t, but since macros are limited to a few K it doesn't matter.

    Would so someone who knows this better take a look?

    File Added: int.diff

     
  • Juergen Keidel
    Juergen Keidel
    2007-01-03

    Logged In: YES
    user_id=86514
    Originator: YES

    uname -a
    Linux wizant2c 2.6.11.4-21.15-smp-pre #1 SMP Fri Dec 29 17:31:30 CET 2006 x86_64 x86_64 x86_64 GNU/Linux
    gcc version 3.3.5 20050117 (prerelease) (SUSE Linux)

    Hardware is intel core2duo Processor

    First compile gave warnings about casting from/to pointer/int in follwing file:
    util/prefFile.c
    highlight.c
    interpret.h
    parse.y
    userCmds.c
    interpret.c
    macro.c
    preferences.c

    changing the int to long int let the warnings disappear.
    Running nedit with smart indent resulted in segfault at insertion of first character in a new line.
    This happened in one of the Branch functions in interpreter.c at a statement like:
    addr = PC + (int)*PC;
    or:
    PC += (long int)*PC;

    replacing these by a sequence like:
    long int increm;

    increm = (((long int)*PC)<<32)>>32; /* make a correct value */
    addr = PC + increm;

    in branchTrue, branch, branchFalse, arrayIter
    solved the problem.

    In addition I had to change the makefile.linux,
    LIBS= -L/usr/X11R6/lib64 -Wl,-Bstatic -lXm -Wl,-Bdynamic -lXp -lXpm -lXext -lXt -lSM -lICE -lX11 -lm

    to link with the correct library.

    No Its running on 64-Bit
    The changed files are attached as changed.tgz

    File Added: changed.tgz

     
  • Juergen Keidel
    Juergen Keidel
    2007-01-03

    Logged In: YES
    user_id=86514
    Originator: YES

    From the file int.diff I see, that my Version of interpreter.c is old.
    Is there any newer release available?

     
  • Scott Tringali
    Scott Tringali
    2007-01-03

    • assigned_to: nobody --> tringali
    • status: open --> open-accepted
     
  • Scott Tringali
    Scott Tringali
    2007-01-03

    Logged In: YES
    user_id=11321
    Originator: NO

    Thanks for the info.

    If the patch doesn't apply you probably have a really old version. What version of nedit are you running? That file hasn't changed a whole lot so I'd expect it to still apply.

    The patch is based of CVS as of a few days ago. You can get that here:
    http://www.nedit.org/ftp/snapshot/nedit-latest-sources-HEAD.tar.gz

    It also probably will apply to the latest release:
    http://www.nedit.org/ftp/v5_5/

    The suggested fix is platform-specific as the bug in the first place, so it's just replacing a bug with another. Long is not guaranteed to hold a pointer any more than int is.

     
  • Scott Tringali
    Scott Tringali
    2007-01-11

    Logged In: YES
    user_id=11321
    Originator: NO

    Still can't repro this. Please send output of nedit -V, or verify that the patch works for you.

     
  • Juergen Keidel
    Juergen Keidel
    2007-01-11

    Logged In: YES
    user_id=86514
    Originator: YES

    I downloaded nedit-5.5-src.tar.bz2
    compiling it still results in a lot of compiler warning of type:
    warning: cast from pointer to integer of different size

    For the linker I had to change in makefile.linux
    LIBS= -L/usr/X11R6/lib64 -Wl,-Bstatic -lXm -Wl,-Bdynamic -lXp -lXpm -lXext -lXt -lSM -lICE -lX11 -lm

    to get the right libraries.

    ignoring the compiler warnings nedit works

    my actual Version of nedit:
    nedit -V
    NEdit 5.5
    Sep 30, 2004

    Built on: Linux, x86-64, GNU C
    Built at: Jan 6 2007, 13:39:59
    With Motif: 2.2.3 [@(#)Motif Version 2.2.3]
    Running Motif: 2.2 [unknown]
    Server: The X.Org Foundation 60802000
    Visual: 24-bit TrueColor (ID 0x21, Default)
    Locale: en_US

    the downloaded version:
    source/nedit -V
    NEdit 5.5
    Sep 30, 2004

    Built on: Linux, x86-64, GNU C
    Built at: Jan 11 2007, 17:31:35
    With Motif: 2.2.3 [@(#)Motif Version 2.2.3]
    Running Motif: 2.2 [unknown]
    Server: The X.Org Foundation 60802000
    Visual: 24-bit TrueColor (ID 0x21, Default)
    Locale: en_US

    I modified the actual version.

    The critical statements are in interpreter.c within the branch functions. Changing the cast of PC from int to long int
    to get rid of the compiler warnings will result in a segfault (smart indent active) when inserting a character at a new line.

    The problems are occuring only on a 64-bit system

     
  • Scott Tringali
    Scott Tringali
    2007-01-11

    Logged In: YES
    user_id=11321
    Originator: NO

    Please try the attached patch in this bug report and let us know if it works.

    The suggested fix will not work on other systems, it's the same bug, but shifts the problem onto other platforms.

     
  • Juergen Keidel
    Juergen Keidel
    2007-01-14

    Logged In: YES
    user_id=86514
    Originator: YES

    I tried to analyse the problem a bit deeper and here ist the test program with the results:
    #include <stdlib.h>
    typedef int (*Inst)(void);
    int main(void){

    int i , k = 7 ,ni;
    Inst *PC;

    long int l;

    PC =(Inst *)&k;

    i=(int)*PC;
    l = (long int)*PC;
    ni = (int)(long int)*PC;
    printf("k: %x p:%p i=%x l=%lx ni:%x \n",k,PC,i,l,ni);
    }

    /* result with cc -m32 -g test.c :
    k: 7 p:0xffffc260 i=7 l=7 ni:7 */

    /* result on 64-Bit system with cc -g test.c :
    k: 7 p:0x7fffffffc3b8 i=7 l=700000007 ni:7 */

    Its clear to see, that "i" gets a wrong result on a 64-Bit system. If this value is used to increment the pointer now it must lead to a segfault!

    effected functions in interpret.c:
    branch(void)
    branchTrue(void)
    branchFalse(void)
    arrayIter(void)
    If compiled on 32 bit system, the value is Ok, no compiler errors.

    I tried the attached path and found no difference

    It really seems, that the problem will occur only, if one tries to solve the ugly compiler warning about wrong casting an has smart indent active on a 64-Bit system.

     
  • Tony Balinski
    Tony Balinski
    2007-01-19

    Logged In: YES
    user_id=618141
    Originator: NO

    Are we done with this now? The new union version of the instruction was checked into the dev head 12jan2007 (with a wee fix on the 15th). Works fine for me but I have classic 32bit pointers! Juergen, did you get an opportunity to try the new version out? (You can download the dev source tarball at http://www.nedit.org/ftp/snapshot/nedit-latest-sources-HEAD.tar.gz in case you didn't know.)

     
  • Bert Wesarg
    Bert Wesarg
    2007-01-19

    Logged In: YES
    user_id=122956
    Originator: NO

    i use the CVS on my opteron workstation since the commits, and doesn't have any crashes

    cheers

     
  • Juergen Keidel
    Juergen Keidel
    2007-01-20

    Logged In: YES
    user_id=86514
    Originator: YES

    The version head 12jan2007 runs now without problems, even while there are some compiler warnings of:
    cast to pointer from integer of different size
    in the files (file linenumber)
    prefFile.c: 356 , 358
    preferences.c: 2302 , 5154 , 5175 , 5182 , 5182
    userCmds.c: 1119
    macro.c: 2916 , 2957 , 3093 , 3147 , 3672 , 3752
    highlight.c: 515

    basically I think the problem is solved.
    Thank you for the help
    J.Keidel

     
  • Tony Balinski
    Tony Balinski
    2007-01-20

    Logged In: YES
    user_id=618141
    Originator: NO

    Actually I rather object to compilers complaining about explicit casts. I believe the problem encountered here was that the "correction" of the data shortening cast - pointer to int in this case - in instruction decoding (interpret.c) was not matched by a change of the corresponding cast from int to pointer in the instruction generation phase (parse.y). I would have thought that a compiler should not warn about explicit shortening casts - at some point the compiler writer should assume that the programmer knows what he's doing. It may not be the best - or even a good way - to do something, but it is not "wrong".

    That being said, Scott's union is a better way to go. It forces the types to match in both files (as long as the same element of the union is used) and makes the use of the instruction value more explicit. It's good to know that the changes solve the problem and improves the code too.

    Tony

     
  • Scott Tringali
    Scott Tringali
    2007-01-20

    Logged In: YES
    user_id=11321
    Originator: NO

    If Tony likes the change then let's keep it. We'll deal with the other warnings elsewhere. Bumping priority a bit since it was a crash.

     
  • Scott Tringali
    Scott Tringali
    2007-01-20

    • priority: 5 --> 7
    • status: open-accepted --> open-fixed
     
  • Tony Balinski
    Tony Balinski
    2007-03-01

    Logged In: YES
    user_id=618141
    Originator: NO

    Sorry I didn't respond earlier: IMO this fix is fine. As far as I'm concerned, this can be closed.

     
  • Thorsten Haude
    Thorsten Haude
    2007-03-04

    • status: open-fixed --> closed-fixed