search

adventuregamestudio / ags

AGS editor and engine source code
Other
672 stars 159 forks source link

AGS 4.0: replace C arrays in ccScript with std containers #2411

Closed ivan-mogilko closed 3 weeks ago

ivan-mogilko commented 3 weeks ago

This replaces C arrays in ccScript class, and all the related reallocation code, with std containers: std::vectors and std::strings. Adjusts both old and new compilers.

A care had to be taken when fixing parsers, as AGS script's bytecode is based on int32 values for historical reasons. Because of that I had to introduce a helper function codesize_i32() which lets to clearly state the purpose of using "current code size" in assignment to int32 variables.

Started with AGS 4, as it has 2 script compilers, but will backport main part to the ags3 branch too.

ivan-mogilko commented 3 weeks ago

There's something strange with the new compiler, found by this test: https://github.com/adventuregamestudio/ags/blob/dcdfbc658e87de0a38f38e9ea04159f604398c1c/Compiler/test2/cc_bytecode_test_1.cpp#L2309

It appears that at some point the parser overwrites a codebyte element beyond the valid range. The "size" of bytecode array is 6, but parser tries to assign to 10th element.

Testing the current ags4 shows the same. The reason why this works (or "works") is that bytecode array's allocated capacity is larger than the data size, and unlike std::vector, size is not asserted when writing to a raw array.

Running tests built in "release" configuration does not indicate this problem, although I can see thrown exceptions in the output log when running under MSVS.

ivan-mogilko commented 3 weeks ago

Following new compiler's tests cause std::vector assertion (write beyond valid array range): TEST_F(Bytecode1, Ternary5) TEST_F(Compile0, Ternary02) TEST_F(Compile1, CompileTimeConstant2)

I do not yet know if that's a mistake in my refactor, or a mistake in the new compiler. Curiously, when built in "release" configuration, all tests pass successfully, meaning they return expected result(?), and this buffer overflow does not hurt them.

ivan-mogilko commented 3 weeks ago

Ah, so, these errors are present in the new compiler without my changes. Opened #2417 for easier testing.

EDIT: I guess since these are confirmed to be compiler bugs, this PR may be merged (assuming it actually works and compiles game scripts).