View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0003121 | FSSCP | scripting | public | 2014-10-04 00:12 | 2014-10-09 02:23 |
| Reporter | Axem | Assigned To | Goober5000 | ||
| Priority | normal | Severity | crash | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.7.2 RC4 | ||||
| Target Version | 3.7.2 | Fixed in Version | 3.7.2 | ||
| Summary | 0003121: mn.evaluateSEXP is broken | ||||
| Description | If 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 Information | From 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) | ||||
| Tags | No tags attached. | ||||
|
|
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. |
|
|
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 ')'. |
|
|
*shrug* You'll have to talk to whoever wrote that SEXP to understand their thought process on it. |
|
|
"That SEXP"? This is the basic SEXP parsing code. |
|
|
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. |
|
|
RunSEXP is now fixed. I did some hardening of the sexp parsing code and will finish that up soon. |
|
|
SEXP parsing is all fixed up now; parse errors will no longer risk memory violations. |
|
|
With the completion of those two items, marking ticket as fixed. |
|
fs2open: trunk r11110 2014-10-07 22:16 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 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 | ||
| 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 |
