Skip to content

Loading…

Leak of `x` to the global scope in `utils.coffee` #93

Closed
GraemeF opened this Issue · 10 comments

2 participants

@GraemeF

There is a leak of x to the global scope due to jashkenas/coffeescript#2422 at https://github.com/sockjs/sockjs-node/blob/master/src/utils.coffee#L141 thanks to the catch x at https://github.com/sockjs/sockjs-node/blob/master/src/utils.coffee#L11

I don't know coffeescript but it might be as simple as renaming the x used with catch?

@majek
SockJS - WebSocket emulation member

Interesting. Thanks for spotting. Is it an actual bug, or only a quirk?

@GraemeF

In CoffeeScript? It's supposed to take care of declaring all variables with a var for you, so it's a bug (and they've fixed it in what looks like a rewrite of the compiler).

In SockJS it's more of a quirk, but it's a nasty one because it could overwrite whatever is named x in the global scope (which I'm sure will catch a handfull of beginners out!). For me it's a pain because mocha tells me I have a global leak whenever I run my end-to-end tests, unless I declare x as a global, which I'd rather not do. I spent a lot of time tracking down where x was being leaked and I'm sure others will waste that time, too.

In any case, it doesn't seem intentional so I'm hopeful that it's worthy of a fix :wink:

@majek
SockJS - WebSocket emulation member

I may be missing something, but I don't think anything leaks to global namespace. This coffeescript:

x = 0
try
    undefinedfunction()
catch x
    console.log x
console.log x

compiles to:

(function() {
  var x;

  x = 0;

  try {
    undefinedfunction();
  } catch (x) {
    console.log(x);
  }

  console.log(x);

}).call(this);

And when exectuted prints:

$ coffee -c a.coffee
$ node a.js
[ReferenceError: undefinedfunction is not defined]
0

Did I miss something?

Btw: node v0.8.10, coffee 1.2.0

@majek majek added a commit that referenced this issue
@majek majek #93 - cosmetic - unify "catch" statement
And always try to use "x" as a parameter.
8f7ed67
@majek majek added a commit that referenced this issue
@majek majek #93 - cosmetic
null is not required
f83f99f
@majek
SockJS - WebSocket emulation member

Right, this was already fixed in 'dev' branch: 8f5bc2b

@GraemeF

@majek That's correct, but without the initial x = 0 you don't get a var x which then leaks x.

@GraemeF

Aha! Well spotted; 8f5bc2b does indeed look like the required workaround. The good news is the CoffeeScript compiler has a fix coming too, whenever their next major release is done.

@GraemeF GraemeF closed this
@majek
SockJS - WebSocket emulation member

This is not correct, nothing is leaked. a.coffee:

try
    undefinedfunction()
catch x
    console.log x
console.log x

Result:

(function() {

  try {
    undefinedfunction();
  } catch (x) {
    console.log(x);
  }

  console.log(x);

}).call(this);

Running:

$ coffee -c a.coffee
$ node a.js
[ReferenceError: undefinedfunction is not defined]

/home/marek/sockjs-node/a.js:9
  console.log(x);
              ^
ReferenceError: x is not defined
@GraemeF

Well there is still no declaration of x - maybe I'm overstating it as a global leak, but mocha considers it a problem (as do the CoffeeScript devs).

@GraemeF
try
    undefinedfunction()
catch x
    console.log x

x = 42
console.log x

compiles to:

(function() {

  try {
    undefinedfunction();
  } catch (x) {
    console.log(x);
  }

  x = 42;

  console.log(x);

}).call(this);

You see there is no var x? Anyway, don't worry about it, it's already been fixed in 2 places :smile:

@majek
SockJS - WebSocket emulation member

Ok, now I understand it. Thanks.

@majek majek added a commit that referenced this issue
@majek majek #93 - cosmetic
null is not required
efb758f
@majek majek added a commit that referenced this issue
@majek majek #93 - cosmetic - unify "catch" statement
And always try to use "x" as a parameter.
b28bd76
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.