HaxeFoundation / hxcpp

Runtime files for c++ backend for haxe
Other
295 stars 181 forks source link

Invalid cast succeeds and leads to crash #751

Open bjitivo opened 5 years ago

bjitivo commented 5 years ago

Here is a simple class to reproduce the issue:

class HxcppBug
{
    public static function main()
    {
        var s : String = "";

        var c = try cast(s, Interface) catch (e : Dynamic) null;

        if ((c == null) || c.foo) {
            trace("Impossible");
        }
    }
}

private interface Interface
{
    var foo(get, null) : Bool;
}

This program crashes. c should be evaluated to null, but is instead just assigned from s. Then the call to the get_foo() function crashes since there is no such function on String.

bjitivo commented 5 years ago

This fails for the neko target too so I am thinking that this is actually a haxe front-end bug, not an hxcpp-specific bug. It appears that haxe now just silently accepts invalid casts. I am using haxe 3.4.7 by the way.

bjitivo commented 5 years ago

Whoops I am wrong. My test case was bad. Here is a better version:

class HxcppBug
{
    public static function main()
    {
        var s : String = "";

        if (Std.is(s, Interface)) {
            trace("It is an Interface");
        }

        var c = try cast(s, Interface) catch (e : Dynamic) null;

        if (c == null) {
            trace("OK");
        }
        else if (c.foo) {
            trace("Impossible");
        }
    }
}

private interface Interface
{
    var foo(get, null) : Bool;
}
bjitivo commented 5 years ago

The new version of the test works correctly for neko target but fails for hxcpp target.

ncannasse commented 5 years ago

@hughsando can you look into it?

bjitivo commented 5 years ago

This is a very significant issue for us as we cannot upgrade to the 3.4.7 haxe compiler with this bug since it affects a large number of lines of our code. Thanks.

bjitivo commented 5 years ago

I am trying to write a macro to fix this temporarily. But I am having a hard time. What is wrong with the following?

    static function processExpr(e : Expr) : Expr
    {
        // xxx find the cast(a, b) expressions and rewrite them to:
        // Std.is(a, b) ? cast(a, b) : throw invalid cast exception
        switch (e) {
        // var f : t = cast e;
        case { expr : ECast(et, null), pos : pos }:
            return { expr : ECast(et.map(processExpr), null), pos : pos };
        // var f = cast(e, t)
        case { expr : ECast(et, t), pos : pos}:
            var econd : Expr = macro @:pos(pos) Std.is($v{et}, $v{t});
            var eif : Expr = { expr : ECast(et.map(processExpr), t),
                               pos : et.pos };
            var eelse : Expr = macro @:pos(pos) throw "Bad cast";
            return { expr : ETernary(econd, eif, eelse), pos : pos };
        default:
            return e.map(processExpr);
        }
    }

I get errors that look like this when I use the above macro:

/sandbox/b-haxetools-wb-haxe-compilerupdate-mainline/working/dev-host/root/haxe/lib/openfl-tivo-stb/0,0,0/openfl/display/Stage.hx:1179: characters 26-51 : Object requires fields: min, max, file /sandbox/b-haxetools-wb-haxe-compilerupdate-mainline/working/dev-host/root/haxe/lib/openfl-tivo-stb/0,0,0/openfl/display/Stage.hx:1179: characters 26-51 : For function argument 'e' /sandbox/b-haxetools-wb-haxe-compilerupdate-mainline/working/dev-host/root/haxe/lib/openfl-tivo-stb/0,0,0/openfl/display/Stage.hx:1178: lines 1178-1180 : Missing return: Int App1.hx:44: characters 37-38 : Object requires fields: min, max, file App1.hx:44: characters 37-38 : For function argument 'e1' App1.hx:44: characters 8-86 : Void should be _App1.Interface

Something about "Object requires fields: min, max, file" ... ???

Simn commented 5 years ago

What code is Stage.hx:1179 referring to? The error suggests that there's some mixup of haxe.macro.Position objects, but I can't immediately tell where that comes from.

Simn commented 5 years ago

@hughsando: Something is going wrong with safe-casts in that they are not generated at all:

class Main {
    static public function main() {
        cast(whatever(), I);
        cast(whatever(), C);
    }

    static function whatever():Dynamic return "";
}

interface I { }
class C { }

Generated cpp:

void Main_obj::main(){
                HX_STACKFRAME(&_hx_pos_e47a9afac0942eb9_2_main)
HXLINE(   2)
HXLINE(   3)        ::Main_obj::whatever();
HXLINE(   4)        ::Main_obj::whatever();
                }

The cast is in the dump file, so this must go wrong at generator level. At first I thought this problem was specific to interfaces, but it reproduces for classes too as you can see.

Any idea what's going on here?

ncannasse commented 5 years ago

@bjitivo what is the latest know Haxe version you are using that works well regarding this issue? We can try to bissect from here

Simn commented 5 years ago

Ok I see, there are two problems:

The second one is the original issue here.

ncannasse commented 5 years ago

I guess the first one is some kind of optimization for casts that are reaching toplevel for some reason. I don't think that a good idea because it changes exception behavior. The second one might also be an optimization if the class matches the interface already.

Are these not problems because we unify the TMono returned by Dynamic with the casted type ? In which case some optimizer somewhere might see these as no-ops and optimize them out.

ncannasse commented 5 years ago

@bjitivo here's a macro that allows to replace the invalid interface casts. we are looking at fixing that into the compiler, tell us if a 3.4.8 release would help you -- we might want to be sure that all fixes are applied before.

import haxe.macro.Context;

class Macro {

    static function loop( e : haxe.macro.Expr ) {
        haxe.macro.ExprTools.iter(e,loop); 
        switch( e.expr ) {
        case ECast(v,t):
            var ct = Context.resolveType(t,e.pos);
            switch( Context.follow(ct) ) {
            case TInst(c,_) if( c.get().isInterface ):
                var pack = switch( t ) {
                case TPath(p): p.pack.concat([p.name]);
                default: Context.error("Unrecognized interface path",e.pos);
                }
                var vt : haxe.macro.Expr = { expr : EConst(CIdent(pack.shift())), pos : e.pos };
                while( pack.length > 0 )
                    vt = { expr : EField(vt,pack.shift()), pos : e.pos };
                e.expr = (macro Std.is($v,$vt) ? (cast $v : $t) : throw "Cast error").expr;
            default:
            }
        default:
        }
    }

    public static function build() {
        var fields = Context.getBuildFields();
        for( f in fields )
            switch( f.kind ) {
            case FFun(f):
                loop(f.expr);
            default:
            }
        return fields;
    }

}
bjitivo commented 5 years ago

Yes a 3.4.8 release would help us, as we cannot go forward to 4.0 as we had even more problems (mostly surrounding just getting the haxe compiler to be compiled, the additional ocaml dependencies introduced in 4.0 are a real problem as they are not available pre-packaged on the Linux distributions we use, and they cannot easily be built from source).

bjitivo commented 5 years ago

But we may have more issues to report with 3.4.7, so hold off on the 3.4.8 for now.

ncannasse commented 5 years ago

@bjitivo ok. I've just slightly improved the macro by doing (cast $v : $t) which gives a better type checking

ncannasse commented 5 years ago

@bjitivo regarding your ocaml issues, it's not urgent I guess but what are the libraries that cannot be compiled ? are you using opam ?

ncannasse commented 5 years ago

You can find our automated builds here: http://hxbuilds.s3-website-us-east-1.amazonaws.com/builds/haxe/

stevemayhew commented 5 years ago

I cannot build from source on my workstation. Here's the versions I have:

SteveMayhewsMacBook:haxe smayhew$ git status
HEAD detached at 3.4.7
nothing to commit, working tree clean
SteveMayhewsMacBook:haxe smayhew$ git remote -v
origin  https://github.com/HaxeFoundation/haxe.git (fetch)
origin  https://github.com/HaxeFoundation/haxe.git (push)
tivo    https://github.com/TiVo/haxe (fetch)
tivo    https://github.com/TiVo/haxe (push)
SteveMayhewsMacBook:haxe smayhew$ git status
HEAD detached at 3.4.7
nothing to commit, working tree clean
SteveMayhewsMacBook:haxe smayhew$ git submodule status
 a494d8be523e26fcf875e2c33915808dc221e17a extra/haxelib_src (3.3.0-195-ga494d8b)
 ab5be31c6dd1fcd761c2ba16c5d767bcf6792490 libs (remotes/origin/3.4_bugfix)
SteveMayhewsMacBook:haxe smayhew$ 
SteveMayhewsMacBook:haxe smayhew$ ocaml -version
The OCaml toplevel, version 4.02.2
SteveMayhewsMacBook:haxe smayhew$ opam list
# Installed packages for system:
base-bigarray           base  Bigarray library distributed with the OCaml compiler
base-bytes              base  Bytes library distributed with the OCaml compiler
base-ocamlbuild         base  OCamlbuild binary and libraries distributed with the OCaml compiler
base-threads            base  Threads library distributed with the OCaml compiler
base-unix               base  Unix library distributed with the OCaml compiler
biniou                1.0.12  Binary data format designed for speed, safety, ease of use and backward compatibility as protocols evolve
camlp4           4.02+system  Camlp4 is a system for writing extensible parsers for programming languages
conf-libpcre               1  Virtual package relying on a libpcre system installation.
conf-m4                    1  Virtual package relying on m4
conf-pkg-config          1.0  Virtual package relying on pkg-config installation.
conf-which                 1  Virtual package relying on which
cppo                   1.5.0  Equivalent of the C preprocessor for OCaml programs
easy-format            1.2.0  High-level and functional interface to the Format module of the OCaml standard library
gen                      0.4  Simple and efficient iterators (modules Gen and GenLabels).
merlin                 2.5.4  Editor helper, provides completion, typing and source browsing in Vim and Emacs
ocamlbuild                 0  Build system distributed with the OCaml compiler since OCaml 3.10.0
ocamlfind              1.7.1  A library manager for OCaml
pcre                   7.2.3  pcre-ocaml - bindings to the Perl Compatibility Regular Expressions library
ppx_tools         5.0+4.02.0  Tools for authors of ppx rewriters and other syntactic tools
sedlex                1.99.3  ppx-based unicode-friendly lexer generator
yojson                 1.3.3  Yojson is an optimized parsing and printing library for the JSON format 
SteveMayhewsMacBook:haxe smayhew$ 
SteveMayhewsMacBook:haxe smayhew$ make
make -C libs/extlib OCAMLOPT=ocamlopt OCAMLC=ocamlc native
ocamlopt -g -a -o extLib.cmxa enum.mli bitSet.mli dynArray.mli extArray.mli extHashtbl.mli extList.mli extString.mli global.mli rbuffer.mli IO.mli option.mli pMap.mli std.mli uChar.mli uTF8.mli base64.mli unzip.mli refList.mli optParse.mli dllist.mli multiArray.mli sMap.mli enum.ml bitSet.ml dynArray.ml extArray.ml extHashtbl.ml extList.ml extString.ml global.ml rbuffer.ml IO.ml option.ml pMap.ml std.ml uChar.ml uTF8.ml base64.ml unzip.ml refList.ml optParse.ml dllist.ml multiArray.ml sMap.ml extLib.ml
File "bitSet.ml", line 23, characters 40-53:
...
ocamlc  -I pcre pcre_stubs.c
pcre_stubs.c:56:10: fatal error: 'pcre.h' file not found
#include "pcre.h"
         ^~~~~~~~
1 error generated.

Once I edit the make file to search /opt/local/... for PCRE, haxe builds clean. But the compiler generated fails to compile haxe hello world:

SteveMayhewsMacBook:haxe smayhew$ ./haxe -v
Haxe Compiler 3.4.7 - (C)2005-2017 Haxe Foundation
 Usage : haxe -main <class> [-swf|-js|-neko|-php|-cpp|-cppia|-as3|-cs|-java|-python|-hl|-lua] <output> [options]

compile and run:

SteveMayhewsMacBook:haxe smayhew$ haxe -main Main --interp
/usr/local/lib/haxe/std/neko/Boot.hx:74: characters 11-30 : Invalid package : std should be <empty>
std/Type.hx:300: characters 13-27 : Invalid package : std should be <empty>
std/Type.hx:301: characters 12-25 : Invalid package : std should be <empty>
std/Type.hx:53: characters 37-46 : Invalid package : std should be <empty>
std/Type.hx:83: characters 61-67 : Invalid package : std should be <empty>
std/Type.hx:148: characters 65-79 : Invalid package : std should be <empty>
ncannasse commented 5 years ago

@stevemayhew can you try compiling from outside the "haxe" directory?

hughsando commented 5 years ago

I will have a look at this. It is related to https://github.com/HaxeFoundation/haxe/issues/6482, although there are a few extra checks I should do in the unsafe case.

bjitivo commented 5 years ago

Do the recent commits fix the issue?

Can we get these applied to the 3.4.7 branch?

stevemayhew commented 5 years ago

@stevemayhew can you try compiling from outside the "haxe" directory?

Interesting... that worked for the Neko target just fine:

stevemayhewsmacbook:opensource smayhew$ export HAXE_STD_PATH=~/opensource/haxe/std/ stevemayhewsmacbook:opensource smayhew$ ~/opensource/haxe/haxe -main Main --interp Main.hx:3: Hello World

Simn commented 5 years ago

Do the recent commits fix the issue? Can we get these applied to the 3.4.7 branch?

Yes and yes, though we'll have to put some efforts into backporting it properly. I've opened https://github.com/HaxeFoundation/haxe/issues/7662 to keep track of that.

Interesting... that worked for the Neko target just fine:

This is an old problem which IIRC comes from the fact that Haxe adds "." as a class path, and then is confused because it comes across the "./std" directory if executed from the Haxe root directory.

stevemayhew commented 5 years ago

Ah.. So the gotcha is if your class path is a parent of HAXE_STD_LIB? Given the way we construct our build we will have to be careful of that since we don’t ‘install’ haxe compiler in a way that haxe std lib would land up in a system like folder (aka /usr/local/lib). Seems like an easy hole to fall into.

On Dec 10, 2018, at 11:29 AM, Simon Krajewski notifications@github.com wrote:

Do the recent commits fix the issue? Can we get these applied to the 3.4.7 branch?

Yes and yes, though we'll have to put some efforts into backporting it properly. I've opened HaxeFoundation/haxe#7662 https://github.com/HaxeFoundation/haxe/issues/7662 to keep track of that.

Interesting... that worked for the Neko target just fine:

This is an old problem which IIRC comes from the fact that Haxe adds "." as a class path, and then is confused because it comes across the "./std" directory if executed from the Haxe root directory.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HaxeFoundation/hxcpp/issues/751#issuecomment-445941593, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYS-A5-2774uQWY0zDOp7T_gmRYx2Fgks5u3rYfgaJpZM4Y8Y77.

scottwalters commented 5 years ago

Apparently OT for this ticket, but also seeing the issue reported in https://github.com/HaxeFoundation/hxcpp/issues/751#issuecomment-444970698 above:

gmake[1]: Entering directory '/home/tgz/haxe/libs/pcre'
ocamlc -safe-string  -I pcre pcre_stubs.c
pcre_stubs.c:56:10: fatal error: 'pcre.h' file not found
#include "pcre.h"
         ^~~~~~~~
1 error generated.
gmake[1]: *** [Makefile:20: pcre_stubs.o] Error 2
gmake[1]: Leaving directory '/home/tgz/haxe/lib

At least a doc fix to https://github.com/HaxeFoundation/haxe/blob/development/extra/BUILDING.md would be nice. It currently says:

Compile

In the checked out Haxe source directory,

# On Unix
make

# On Windows (Cygwin)
make -f Makefile.win