2018-05-25 23:28 EDT


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0003121FSSCPscriptingpublic2014-10-08 22:23
ReporterAxem 
Assigned ToGoober5000 
PrioritynormalSeveritycrashReproducibilityalways
StatusresolvedResolutionfixed 
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.
Attached Files

-Relationships
+Relationships

-Notes

~0016322

Goober5000 (administrator)

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.

~0016323

MageKing17 (developer)

Last edited: 2014-10-03 22:51

View 3 revisions

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 ')'.

~0016324

Goober5000 (administrator)

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

~0016325

MageKing17 (developer)

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

~0016326

Goober5000 (administrator)

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.

~0016331

Goober5000 (administrator)

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

~0016332

Goober5000 (administrator)

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

~0016333

Goober5000 (administrator)

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

+Related Changesets

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