Source Code Project Mantis - FSSCP
View Issue Details
0003121FSSCPscriptingpublic2014-10-03 20:122014-10-08 22:23
Assigned ToGoober5000 
PlatformOSOS Version
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.")

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.
Attached Files

2014-10-03 21:52   
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.
2014-10-03 22:22   
(Last edited: 2014-10-03 22:51)
What is the purpose of these two lines in get_sexp_main()?

Assert(*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 ')'.

2014-10-04 22:03   
*shrug* You'll have to talk to whoever wrote that SEXP to understand their thought process on it.
2014-10-04 22:53   
"That SEXP"? This is the basic SEXP parsing code.
2014-10-05 13:23   
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.
2014-10-07 22:36   
RunSEXP is now fixed. I did some hardening of the sexp parsing code and will finish that up soon.
2014-10-08 22:22   
SEXP parsing is all fixed up now; parse errors will no longer risk memory violations.
2014-10-08 22:23   
With the completion of those two items, marking ticket as fixed.

Issue History
2014-10-03 20:12AxemNew Issue
2014-10-03 20:22Goober5000Target Version => 3.7.2
2014-10-03 21:50Goober5000Additional Information Updatedbug_revision_view_page.php?rev_id=924#r924
2014-10-03 21:52Goober5000Note Added: 0016322
2014-10-03 22:22MageKing17Note Added: 0016323
2014-10-03 22:47MageKing17Note Edited: 0016323bug_revision_view_page.php?bugnote_id=16323#r926
2014-10-03 22:51MageKing17Note Edited: 0016323bug_revision_view_page.php?bugnote_id=16323#r927
2014-10-04 22:03Goober5000Note Added: 0016324
2014-10-04 22:53MageKing17Note Added: 0016325
2014-10-05 13:23Goober5000Note Added: 0016326
2014-10-07 20:52Goober5000Assigned To => Goober5000
2014-10-07 20:52Goober5000Statusnew => assigned
2014-10-07 21:37Goober5000Changeset attached => fs2open trunk r11110
2014-10-07 22:36Goober5000Note Added: 0016331
2014-10-08 22:20Goober5000Changeset attached => fs2open trunk r11113
2014-10-08 22:22Goober5000Note Added: 0016332
2014-10-08 22:23Goober5000Note Added: 0016333
2014-10-08 22:23Goober5000Statusassigned => resolved
2014-10-08 22:23Goober5000Resolutionopen => fixed
2014-10-08 22:23Goober5000Fixed in Version => 3.7.2