Crash on unclosed c-style comments under some circumstances

[XenHat]

using lslint v1.0.9 with Sublimelinter-contrib-lslint and MCPP V.2.7.2:

The following script causes lslint.exe to crash on every lint attempt:

default
{
    state_entry()
    {
        if(llOwnerSay("Hello!"))
        /* c-style comment without matching ending delimiter
        /* a valid c-style comment */
            integer linkCount = 0;
        }
    }
}

This is as generic as I could reproduce it (I spoke about this issue with @Sei-Lisa several months ago but the specifics of this bug and the holidays delayed reproducing it for quite some time).
It would appear that the following sequence is required:

1. A function call (preprocessor or else) inside an if statement (maybe any check?)
2. An improperly closed c-style comment
3. ...Followed by a properly closed one
4. An assignment using either a function call or a literal value/constant. = ; or any other error-producing code will NOT trigger the crash.

Thoughts: Based on the timing and specifics of 4, it appears to me that the crash would happen after validating the specific line, perhaps in some "post" step?

EDIT: Here's a video of the issue (Click the image, cannot embed :( ):

[Sei-Lisa]

Thanks for the repro. I've distilled it to this:

default{timer(){
  if (1) integer i = 0;
}}

That should give an error as it does in the SL compiler, as you can't have a declaration as a single statement, because you need to enclose declarations in a { ... }block, but if the declaration includes an assignment, it crashes before getting to that point. Removing the assignment properly reports ERROR:: ( 2, 10): Declaration needs braces {}.. The syntax errors in your case are all recoverable errors, but since the analysis stops before any error has a chance to be reported, due to the crash, they aren't seen.

Here's the backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x000000000041378c in LLScriptSymbol::set_constant_value (this=0x0, v=0x84c3d0)
    at symtab.hh:50
50	    void                    set_constant_value(class LLScriptConstant *v)   { constant_value = v;       };
(gdb) bt
#0  0x000000000041378c in LLScriptSymbol::set_constant_value (this=0x0, 
    v=0x84c3d0) at symtab.hh:50
#1  0x000000000041669b in LLScriptDeclaration::determine_value (this=0x84c500)
    at values.cc:33
#2  0x00000000004165b3 in LLASTNode::propagate_values (this=0x84c500)
    at values.cc:21
#3  0x000000000041658a in LLASTNode::propagate_values (this=0x84c560)
    at values.cc:16
#4  0x000000000041658a in LLASTNode::propagate_values (this=0x84c620)
    at values.cc:16
#5  0x000000000041658a in LLASTNode::propagate_values (this=0x84c710)
    at values.cc:16
#6  0x000000000041658a in LLASTNode::propagate_values (this=0x84c7f0)
    at values.cc:16
#7  0x000000000041658a in LLASTNode::propagate_values (this=0x7dbbc0)
    at values.cc:16
#8  0x0000000000413466 in main (argc=2, argv=0x7fffffffe328) at lslmini.cc:1083

Note the this = 0x0. Looks like it's trying to access an instance that hasn't been created.

I'll investigate, thanks for the report.

[Sei-Lisa]

This is the obvious fix:

diff --git a/values.cc b/values.cc
index 657ea61..55e9742 100644
--- a/values.cc
+++ b/values.cc
@@ -30,7 +30,8 @@ void LLScriptDeclaration::determine_value() {
    LLASTNode *node = get_child(1);
    if ( node == NULL || node->get_node_type() == NODE_NULL ) return;
    DEBUG( LOG_DEBUG_SPAM, NULL, "set %s const to %p\n", id->get_name(), node->get_constant_value() );
-   id->get_symbol()->set_constant_value( node->get_constant_value() );
+   if (id->get_symbol())
+     id->get_symbol()->set_constant_value( node->get_constant_value() );
 }
 
 void LLScriptExpression::determine_value() {

I haven't looked into why it gets into that situation yet, to know whether that's the correct fix, but if you need an emergency fix, there's that.

i'm too busy to look into it in more detail now. When I have some more time I will look and submit a PR.

[Sei-Lisa]

OK, finally got more time to take a closer look at this.

I'm not sure that the above is the right fix. The problem originates from this snippet:

lslint/lslmini.cc

Lines 242 to 252 in 464a28d

	void LLScriptDeclaration::define_symbols() {
	LLNodeSubType type = get_parent()->get_node_sub_type();
	if ( type == NODE_IF_STATEMENT || type == NODE_FOR_STATEMENT
	|| type == NODE_DO_STATEMENT || type == NODE_WHILE_STATEMENT) {
	ERROR( HERE, E_DECLARATION_NOT_ALLOWED );
	return;
	}
	LLScriptIdentifier *identifier = (LLScriptIdentifier *)get_children();
	identifier->set_symbol( new LLScriptSymbol(identifier->get_name(), identifier->get_type(), SYM_VARIABLE, SYM_LOCAL, identifier->get_lloc()) );
	define_symbol(identifier->get_symbol());
	}

Note the return statement at line 247. That's what is preventing the symbol from being defined, causing the crash in values.cc because it's always assumed to exist (which is a reasonable assumption). If I remove it, I get this output from the above test case instead:

ERROR:: (  2, 26): Declaration needs braces {}.
 WARN:: (  2, 34): variable `i' declared but never used.
TOTAL:: Errors: 1  Warnings: 1

I would consider the latter to be more correct, but I'd appreciate opinions.

[Sei-Lisa]

When just removing the return, it fails one testcase, scripts/scope3.lsl which goes like this:

test() { }
default {
    state_entry() {
        integer test = 1; // works fine
        test();
        test = 2;
        if (llGetAttached()) integer test; // $[E10036], but no E10001
    }
}

With the patch, E10001 is also thrown. E10001 is the duplicate identifier error. The scope rules mandate that a new scope is only created when opening braces and when declaring function parameters, therefore the second integer test is in fact in the same scope as the first, meaning it's indeed duplicate. So I think that the assumption of this testcase is wrong because it's correct to throw E10001, and therefore the testcase should be corrected to reflect this.

If everyone agrees, the PR is ready.

[Sei-Lisa]

Sorry for the spam, but I think I should throw this in for consideration. The question is one of philosophy: should the symbol be defined when the declaration is in itself causing an error?

The assumption of the test case is that the symbol should not be defined, i.e. since the declaration is an error, act as if it does not exist. That would lead to my first patch: leave the return in place and check for null before accessing the symbol, because it may not have been created.

But I think that that's poor error recovery. The situation in which this error is most likely to happen is, in my opinion, forgetting the braces. If the symbol is not defined, all subsequent references to it are going to fail, causing errors that would not happen if the symbol was defined. The error message for this case already calls attention to the lack of braces; the rest of errors may be distracting.

That's why I advocate for letting the definition proceed anyway. But if someone thinks otherwise please tell.

[XenHat]

I personally use lslint (or did, until SublimeLinter4 happened and everything broke in a way I simply cannot fix because they removed the functions I depend to make the linter work......) to help me find errors in my code, so errors caused by other errors are something that always get in the way for me. I'd rather have the linter straight up stop processing when such an error exist (except in the case of #define , we went over this already) than give me 35 errors to ignore and figure which one is the one to fix.

[Sei-Lisa]

Thanks for your feedback. Stopping at the first error is the easiest way to go (which is why LL chose that approach and I mimicked it in my optimizer), but it's not what lslint is about. Error recovery consists of trying to second-guess the author's intentions in case an error is found, attempting to act as if the mistake that led to an error is not there, so that hopefully no errors that are directly caused by that one are reported. Computer programs are bad at second-guessing, though, so they must follow predefined strategies and that's why error recovery is a difficult topic.

In this particular case, it seems like the most likely cause for bumping into that error is failing to open the braces of a larger block. I may be wrong on that assessment, but it seems clear that if that's the case, letting the declaration proceed is the best strategy.

I'll submit the PR based on that, then.