[Error] same sub-function & variable name inside a same function

[gogo9th]

Hi,

I really need urgent help on this... Jalangi fails to instrument the following code which is regularly runnable by NodeJS:

function a ()
{  var t;
   function t() {}
}

Jalangi outputs:

SyntaxError: Identifier 't' has already been declared

Could you please give me an easy instruction on how to fix this bug in Jalangi's source code? Thanks very much for your help.

[msridhar]

Thanks for reporting this bug. I am not too sure how easy it would be to fix. @ksen007 do you have any thoughts or ideas here?

[gogo9th]

Jalangi intruments the above code as follows:

                        try {
                            J$.Fe(25, arguments.callee, this, arguments);
                            function t() {
                                jalangiLabel0:
                                    while (true) {
                                        try {
                                            J$.Fe(9, arguments.callee, this, arguments);
                                            arguments = J$.N(17, 'arguments', arguments, 4);
                                        } catch (J$e) {
                                            J$.Ex(81, J$e);
                                        } finally {
                                            if (J$.Fr(89))
                                                continue jalangiLabel0;
                                            else
                                                return J$.Ra();
                                        }
                                    }
                            }
                            arguments = J$.N(33, 'arguments', arguments, 4);
                            t = J$.N(49, 't', J$.T(41, t, 12, false, 9), 0);
                            var t;
                        } catch (J$e) {
                            J$.Ex(97, J$e);

and the error occurs because 'var t' and 'function t' are declared within the same block. However, if these two are declared within the same function, they don't raise an error. Therefore, the above instrumentation should be modified as follows:

                        try {
(function(){
                            J$.Fe(25, arguments.callee, this, arguments);
                            function t() {
                                jalangiLabel0:
                                    while (true) {
                                        try {
                                            J$.Fe(9, arguments.callee, this, arguments);
                                            arguments = J$.N(17, 'arguments', arguments, 4);
                                        } catch (J$e) {
                                            J$.Ex(81, J$e);
                                        } finally {
                                            if (J$.Fr(89))
                                                continue jalangiLabel0;
                                            else
                                                return J$.Ra();
                                        }
                                    }
                            }
                            arguments = J$.N(33, 'arguments', arguments, 4);
                            t = J$.N(49, 't', J$.T(41, t, 12, false, 9), 0);
                            var t;
}())
                        } catch (J$e) {
                            J$.Ex(97, J$e);

This does run without an error. So, I think Jalangi should enclose the global scope and every function scope with (function(){ ... }()).

[msridhar]

Thanks for the explanation! Yes, it seems the issue here is that Jalangi is moving function statements into a nested scope from the top-level scope. I think your idea for an initial fix is reasonable. I think you'd need to introduce the fix at two places in esnstrument.js:

For script bodies:

jalangi2/src/js/instrument/esnstrument.js

Line 954 in 7f0be09

"function n() { jalangiLabel" + l + ": while(true) { try {" + RP + "1} catch(" + JALANGI_VAR +

For function bodies:

jalangi2/src/js/instrument/esnstrument.js

Line 979 in 7f0be09

"function n() { jalangiLabel" + l + ": while(true) { try {" + RP + "1} catch(" + JALANGI_VAR +

Could you go ahead and test out your proposed fix to see if it works? If it works for you, and you open a PR with the fix and your test case, that would be great.

[gogo9th]

I replaced both lines as follow:

                "function n() { jalangiLabel" + l + ": while(true) { try { (function() {" + RP + "1 }()) } catch(" + JALANGI_VAR +

And it works for this sample code:

function a ()
{  var t;
   function t() {}
}

But strangely enough, does not work for this:

var t;
function t() {} 

[msridhar]

Now that I think about it, wrapping the top-level script body in an anonymous function is not a good idea, since on the web, variables declared at the top level are globals that are visible from other scripts. So you'll need another solution there. Or, if the solution for function bodies is good enough for your use case, that might be a good intermediate step for now.

[gogo9th]

Yes, this is the problem I actually just encountered. I started to see many red errors when I tested this on the web...

[msridhar]

Are you running into this problem at the top level of many scripts? I would expect this problem to come up quite rarely.

[gogo9th]

I see this problem for jQuery

[msridhar]

Can you link to the problematic code?

[gogo9th]

I am sorry, but because my version of Jalangi is connected to Puppeteer, it's little complex to show them here... But basically, global objects such as JQuery imported from the web's jquery.js raise an undefined error.

I think this issue could be resolved if we enforce Jalangi to rewrite such undefined objects as window.JQuery instead of just JQuery (we apply this rule only if Jalangi's read / write callback detects that the target object is undefined). What do you think?

[msridhar]

So sorry for my slow response. I see how jQuery would cause an issue with your solution. What I was wondering was are you seeing this issue of a variable and function statement with the same name in the global scope, in function scopes, or both? I was hoping it was only happening in function scopes, so you could just not apply your fix at the global scope for now.

I think your solution of using window.jQuery is reasonable to unblock you. In terms of merging into the main Jalangi2 repo, I'd like to think more about whether there is a less disruptive solution, as I would expect this duplicate name issue is rare in practice.

[gogo9th]

I am really sorry for my late reply. I had a car accident and was trying to recover... Now I am back.

I really want to solve this issue of error for re-definition. As I already tried, enclosing the definitions with a dynamic function is not a good solution, because this causes a scope issue. Do you have any good solution in mind?

[msridhar]

Sorry to hear about your accident, and glad that you've recovered!

As noted originally, this problem arises when there are both a variable and a function statement with the same name defined at the top-level scope of a function. In such cases, Jalangi causes a problem since it moves the function statement within a nested scope. You proposed wrapping the nested scope in an anonymous function. This fix caused issues at the global scope level, since it effectively hides global variables.

My question was, is the issue with a duplicate variable and function statement with the same name appearing for you at the top level of scripts? If not, you can just apply the anonymous function solution within functions and you are done. If it is appearing at the top level, we'd need to think more. And it'd be great to see an example of such code.

[gogo9th]

Thanks a lot for your worries, I am feeling much better now.

My question was, is the issue with a duplicate variable and function statement with the same name appearing for you at the top level of scripts?

Yes, this happens in https://facebook.com. The code is very complex, but I narrowed down the problematic part, and the issue is because both 'var V' and 'function V' are declared in the same global scope and other scripts try to access them...

[gogo9th]

I think the possible solution is that we wrap the code in the 'wrapScriptBodyWithTryCatch' with an anonymous function, and then right after each function declaration and assignment expression in the top-level scope this anonymous function, we append a new expression in the following format:

this['variable_name'] = variable_name;
this['function_name'] = function_name;

this represents the global scope of the instrumented code. So by doing this, we make every variable/function declared in the anonymous function to be globally accessible and update them whenever their values change.

Do you think this will be a valid solution? If so, I was not sure how/where I am supposed to change the source code to apply this instrumentation...

[msridhar]

I have a possibly simpler temporary workaround. It looks like esnstrument.js supports a configuration function INSTR_TRY_CATCH_ARGUMENTS to configure whether the try-catch blocks are inserted:

jalangi2/src/js/instrument/esnstrument.js

Line 948 in 7f0be09

if (!Config.INSTR_TRY_CATCH_ARGUMENTS || Config.INSTR_TRY_CATCH_ARGUMENTS(node)) {

Could you try uncommenting this line?

jalangi2/src/js/Config.js

Line 53 in 7f0be09

// Config.INSTR_TRY_CATCH_ARGUMENTS = function(ast) {return false; }; // wrap function and script bodies with try catch block and use arguments in J$.Fe. DO NOT USE THIS.

That should disable the try-catch instrumentation altogether. Does that fix your problem? The only catch here is that you will lose the Jalangi callbacks for uncaught exceptions and some stuff related to the arguments array.

If you need that functionality, then I think another solution would be to "hoist" named function statements in front of the inserted try-catch blocks. I think that would be simpler than what you proposed. But it would still be a bit of work.

[gogo9th]

Thanks for the tip. I tried to uncomment that line, but I get this error, then:

Error: IllegalStateException

Also, even if we remove the try-catch block, there is a while loop block outside there, so there wouldn't go away unless we also remove the while loop..

[msridhar]

That flag should remove both the try-catch and the while loop. Too bad it doesn't work right now. Where do you get the IllegalStateException? During instrumentation, or while running the instrumented code?

[gogo9th]

I see.. I get the error during instrumentation:

Failed to instrument function MyFunction()
{
	console.log("MyFunction is called");
//{
		function a(){console.log("a is called")}
		var a;
//}
		a();
		console.log(a);
//	{	
//	}	
}

Error: IllegalStateException
    at getFnIdFromAst (/home/skyer/Desktop/expose_runner/chrome-experiment-result/esnstrument.js:652:19)
    at wrapLiteral (/home/skyer/Desktop/expose_runner/chrome-experiment-result/esnstrument.js:671:37)
    at syncDefuns (/home/skyer/Desktop/expose_runner/chrome-experiment-result/esnstrument.js:1080:37)
    at instrumentScriptEntryExit (/home/skyer/Desktop/expose_runner/chrome-experiment-result/esnstrument.js:1156:28)
    at Object.Program (/home/skyer/Desktop/expose_runner/chrome-experiment-result/esnstrument.js:1467:23)
    at Object.transformAst (/home/skyer/Desktop/expose_runner/chrome-experiment-result/astUtil.js:152:36)
    at transformString (/home/skyer/Desktop/expose_runner/chrome-experiment-result/esnstrument.js:1931:30)
    at Object.instrumentCode (/home/skyer/Desktop/expose_runner/chrome-experiment-result/esnstrument.js:2003:30)
    at Object.Module._extensions..js (/home/skyer/Desktop/expose_runner/chrome-experiment-result/jalangi.js:113:28)

[msridhar]

So it seems the INSTR_TRY_CATCH_ARGUMENTS setting is bitrotted; I didn't see a quick fix for the above issue. I still think further function hoisting is the best solution we've come up with to this problem so far. There is already code to hoist function declarations to the top of a scope, starting here:

jalangi2/src/js/instrument/esnstrument.js

Line 1800 in 7f0be09

function hoistFunctionDeclaration(ast, hoisteredFunctions) {

But this just hoists function declarations to the top of the current scope. We would need to somehow enhance it to hoist the declarations above the generated while loop, try-catch, etc. Also for some reason the current code deliberately does not hoist function declarations above the script-enter and function-enter callbacks. I am not sure why; it seems it could be hoisted further to me, and all tests pass locally if I disable that logic.

Unfortunately I don't have time to fix the above in the near term. It seems pretty doable to me, but not completely trivial. @gogo9th you can either take a crack at this, or try the fix you were thinking about if you think that would be easier.

[gogo9th]

I am very sorry for my late update..

Here I modified esnstrument.js as you suggested:

esnstrument.js

tests.zip

I added the hoistFn_and_ExtractDeclarations() function which hoisters both function declarations and variable declarations to outside the while loop, just above the Jalangi label. This is applied at the end of each wrapScriptBodyWithTryCatch() or wrapFunBodyWithTryCatch() call. When I hoister variables, I do not hoister their initialization.

I hope the code is correct. I tested about 10 tricky test cases I created, and it works fine.

When I hoist, I move function declarations up, but for variable declarations, I only copy them without initialization and put the copied versions up. This means I leave the original variable declarations in their original positions. Would this cause any problem? I do this, because I wanted any variable initializations to happen in the original locations.

[msridhar]

@gogo9th could you open a pull request with your changes? That will make them a lot easier to read. If you could include your tests in the PR, even better.

When I hoist, I move function declarations up, but for variable declarations, I only copy them without initialization and put the copied versions up. This means I leave the original variable declarations in their original positions. Would this cause any problem? I do this, because I wanted any variable initializations to happen in the original locations.

Do you mean that you hoist the declaration, but leave just the initialization in place? That makes sense to me. But if after your transformation, there are two different declarations for the variable, that might cause issues, if they are in different scopes. If you post a PR I can look more carefully at what you did.