View Issue Details

IDProjectCategoryView StatusLast Update
0003121FSSCPscriptingpublic2014-10-09 02:23
ReporterAxem Assigned ToGoober5000  
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.7.2 RC4 
Target Version3.7.2Fixed in Version3.7.2 
Summary0003121: mn.evaluateSEXP is broken
DescriptionIf i try to run...

    if mn.evaluateSEXP("true") then
        ba.error("This was true.")
    end

FS will bring up the error...

Can't find operator rue in operator list
.

Looks like the first character isn't getting read in the string to evaluate.
Additional InformationFrom IRC:

21:08:26 MageKing17: Axem: "true" isn't a SEXP.
21:08:29 MageKing17: "( true )" is.
21:09:02 MageKing17: You walked right over an assertion that the string starts with "(".
21:09:05 Axem: for whatever reason the brackets arent required when using the runSEXP
21:09:19 Axem: so now they're needed for evaluateSEXP?
21:09:26 MageKing17: That's because runSEXP is actually doing this:
21:09:30 MageKing17: snprintf(buf, 8191, "( when ( true ) ( %s ) )", s)
TagsNo tags attached.

Activities

Goober5000

2014-10-04 01:52

administrator   ~0016322

Per the additional information from IRC, it is runSEXP that is incorrect. A SEXP is properly written in enclosed parentheses.

The runSEXP function should be rewritten to not add parentheses, like so:
snprintf(buf, 8191, "( when ( true ) %s )", s)

Unfortunately, reverse compatibility precautions will have to be taken.

MageKing17

2014-10-04 02:22

developer   ~0016323

Last edited: 2014-10-04 02:51

What is the purpose of these two lines in get_sexp_main()?

Assert(*Mp == '(');
Mp++;

EDIT: Okay, after some further reading of the code, I'm confused as to how this test code managed even to fail this gracefully. By all rights, avoding the assumption that all SEXPs are inclosed in parentheses (and skipping the assertions by running on a release build) means it's apparently parsing random memory until it hits a byte it can interpret as ')', at which point it finally gets around to deciding that "rue\0<uninitialized data>)" isn't a valid operator and displaying the error.

EDIT2: And unless I miss my guess, since the check of whether or not the length of the token is too long for TOKEN_LENGTH is an Assert, this could also result in writing things to memory that shouldn't be getting written to, depending on how long it takes before it runs into that closing ')'.

Goober5000

2014-10-05 02:03

administrator   ~0016324

*shrug* You'll have to talk to whoever wrote that SEXP to understand their thought process on it.

MageKing17

2014-10-05 02:53

developer   ~0016325

"That SEXP"? This is the basic SEXP parsing code.

Goober5000

2014-10-05 17:23

administrator   ~0016326

Oh whoops, I thought you were referring to the evaluateSEXP function.

I wouldn't be surprised if that parsing code has some holes in it. I would expect that hasn't been tested heavily because usually its input is simply what FRED outputs in its events.

Goober5000

2014-10-08 02:36

administrator   ~0016331

RunSEXP is now fixed. I did some hardening of the sexp parsing code and will finish that up soon.

Goober5000

2014-10-09 02:22

administrator   ~0016332

SEXP parsing is all fixed up now; parse errors will no longer risk memory violations.

Goober5000

2014-10-09 02:23

administrator   ~0016333

With the completion of those two items, marking ticket as fixed.

Related Changesets

fs2open: trunk r11110

2014-10-07 22:16

Goober5000


Ported: N/A

Details Diff
related to Mantis 0003121, add parentheses if the string passed to runSEXP doesn't have them Affected Issues
0003121
mod - /trunk/fs2_open/code/parse/lua.cpp Diff File

fs2open: trunk r11113

2014-10-08 22:59

Goober5000


Ported: N/A

Details Diff
related to Mantis 0003121, make SEXP parsing more robust, and remove some old debug/unused code in get_sexp_main() Affected Issues
0003121
mod - /trunk/fs2_open/code/parse/sexp.cpp Diff File

Issue History

Date Modified Username Field Change
2014-10-04 00:12 Axem New Issue
2014-10-04 00:22 Goober5000 Target Version => 3.7.2
2014-10-04 01:50 Goober5000 Additional Information Updated
2014-10-04 01:52 Goober5000 Note Added: 0016322
2014-10-04 02:22 MageKing17 Note Added: 0016323
2014-10-04 02:47 MageKing17 Note Edited: 0016323
2014-10-04 02:51 MageKing17 Note Edited: 0016323
2014-10-05 02:03 Goober5000 Note Added: 0016324
2014-10-05 02:53 MageKing17 Note Added: 0016325
2014-10-05 17:23 Goober5000 Note Added: 0016326
2014-10-08 00:52 Goober5000 Assigned To => Goober5000
2014-10-08 00:52 Goober5000 Status new => assigned
2014-10-08 01:37 Goober5000 Changeset attached => fs2open trunk r11110
2014-10-08 02:36 Goober5000 Note Added: 0016331
2014-10-09 02:20 Goober5000 Changeset attached => fs2open trunk r11113
2014-10-09 02:22 Goober5000 Note Added: 0016332
2014-10-09 02:23 Goober5000 Note Added: 0016333
2014-10-09 02:23 Goober5000 Status assigned => resolved
2014-10-09 02:23 Goober5000 Resolution open => fixed
2014-10-09 02:23 Goober5000 Fixed in Version => 3.7.2