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

release
closed-fixed
Program (402)
7
2007-03-04
2006-12-31
Juergen Keidel
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

1 2 3 > >> (Page 1 of 3)
  • 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.

     
1 2 3 > >> (Page 1 of 3)