hxcpp icon indicating copy to clipboard operation
hxcpp copied to clipboard

Invalid cast succeeds and leads to crash

Open bjitivo opened this issue 6 years ago • 25 comments

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 avatar Nov 30 '18 21:11 bjitivo

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 avatar Nov 30 '18 21:11 bjitivo

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 avatar Nov 30 '18 21:11 bjitivo

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

bjitivo avatar Nov 30 '18 21:11 bjitivo

@hughsando can you look into it?

ncannasse avatar Dec 01 '18 20:12 ncannasse

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 avatar Dec 04 '18 17:12 bjitivo

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" ... ???

bjitivo avatar Dec 05 '18 02:12 bjitivo

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 avatar Dec 05 '18 08:12 Simn

@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?

Simn avatar Dec 05 '18 10:12 Simn

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

ncannasse avatar Dec 05 '18 10:12 ncannasse

Ok I see, there are two problems:

  • safe-casts at block-level are ignored
  • safe-casts to interfaces are ignored

The second one is the original issue here.

Simn avatar Dec 05 '18 10:12 Simn

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 avatar Dec 05 '18 11:12 ncannasse

@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;
	}
	
}

ncannasse avatar Dec 05 '18 23:12 ncannasse

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 avatar Dec 05 '18 23:12 bjitivo

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

bjitivo avatar Dec 05 '18 23:12 bjitivo

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

ncannasse avatar Dec 06 '18 08:12 ncannasse

@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 avatar Dec 06 '18 08:12 ncannasse

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

ncannasse avatar Dec 06 '18 08:12 ncannasse

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>

stevemayhew avatar Dec 06 '18 18:12 stevemayhew

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

ncannasse avatar Dec 06 '18 21:12 ncannasse

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.

hughsando avatar Dec 07 '18 02:12 hughsando

Do the recent commits fix the issue?

Can we get these applied to the 3.4.7 branch?

bjitivo avatar Dec 10 '18 19:12 bjitivo

@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

stevemayhew avatar Dec 10 '18 19:12 stevemayhew

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.

Simn avatar Dec 10 '18 19:12 Simn

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 [email protected] 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.

stevemayhew avatar Dec 10 '18 19:12 stevemayhew

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

scottwalters avatar Jun 07 '19 23:06 scottwalters