Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Special char in modules cause error when using require to load from storage #1583

Closed
MaBecker opened this issue Dec 11, 2018 · 14 comments
Closed

Comments

@MaBecker
Copy link
Contributor

The special char is \x00 like used many modules e.g. MQTT.js or SBUS.js.

Sample code test_case_module_sbus.js

@gfwilliams
Copy link
Member

I'm pretty sure that's actually a problem with how you're writing the file.

The file doesn't contain the character \x00, it contains a string with the 4 characters \,x,0,0. When you put those right in a template literal, \x00 gets interpreted immediately and character number 0 gets put there instead.

If you created the string with https://www.espruino.com/File+Converter (or manually double-escaped the \ characters) then you wouldn't have the problem.

@MaBecker
Copy link
Contributor Author

This is how modules are saved to storage

// section of SBUS
00000310  20 20 69 64 78 20 3d 20  64 61 74 61 2e 69 6e 64  |  idx = data.ind|
00000320  65 78 4f 66 28 22 00 0f  22 29 3b 0a 20 20 20 20  |exOf("..");.    |

// section of SBUSemin
00000520  73 74 72 28 63 2b 32 29  2c 63 3d 62 2e 69 6e 64  |str(c+2),c=b.ind|
00000530  65 78 4f 66 28 27 00 0f  27 29 3b 7d 7d 29 2c 61  |exOf('..');}}),a|

// section of SBUSgmin
00000700  62 73 74 72 28 64 2b 32  29 3b 64 3d 63 2e 69 6e  |bstr(d+2);d=c.in|
00000710  64 65 78 4f 66 28 22 00  0f 22 29 7d 7d 29 3b 72  |dexOf("..")}});r|

And this is the output when using the require statements

>SBUS = require('SBUS');
={
  connect: function (uart,rxPin) { ... }
 }
>SBUSemin = require('SBUSemin');
={  }
Uncaught SyntaxError: Got EOF expected '}'
 at line 1 col 475
...b.substr(c+2),c=b.indexOf('�');}}),a;};
                             ^
>SBUSgmin = require('SBUSgmin');
={  }
Uncaught SyntaxError: Got EOF expected '}'
 at line 1 col 452
...c.substr(d+2);d=c.indexOf("�")}});return a}
 

@gfwilliams
Copy link
Member

Yes, that's just what I said above then. I think it's reasonable to expect Espruino to error if it encounters char code 0 when trying to execute code.

If you quote the string properly when saving to storage (so what's in storage actually matches what's in SBUS.js) then it'll work great.

@MaBecker
Copy link
Contributor Author

manually double-escaped the \ characters

>global
={
  s: function () { [native code] },
  SBUS: {
    connect: function (uart,rxPin) { ... }
   },
  SBUSemin: {
    connect: function (c,d) { ... }
   },
  SBUSgmin: {
    connect: function (b,f) { ... }
   }
 }
> 

I thought there is a difference between SBUS and a min version of SBUS that causes the trouble.

@gfwilliams
Copy link
Member

That is strange - I thought in your example code each example broke.

I'm not sure why the unminified version was working fine - unless potentially the format of the if statements meant that the code wasn't actually getting executed.

... or ... could you have just require'd it once before? That might have skipped the re-evaluation

@MaBecker
Copy link
Contributor Author

... or ... could you have just require'd it once before? That might have skipped the re-evaluation

tested again, loads but as you said let's double-escaped the \ characters

Affected files: DigoleBuf.js, DigoleHW.js, MQTT.js and SBUS.js

@MaBecker MaBecker reopened this Feb 23, 2019
@MaBecker
Copy link
Contributor Author

There must be a difference when loading modules from folder (1) and from storage (2), tested with tinyMQTT.

Adding the double-escaped the \ characters is working for (2) but does for (1).

@olliephillips had to remove them again

olliephillips/tinyMQTT@1fd8090#diff-8bbcea8cef04a070ebbbebb8cc491984

@gfwilliams
Copy link
Member

There must be a difference when loading modules from folder (1) and from storage (2)

Maybe I'm misunderstanding...

Can we go with a really simple example - returning a quote?

exports.quote = "\"";

In a module that works. And you're saying that when you do:

Modules.removeAllCached();
require("Storage").write("foo",`exports.quote = "\"";`);
require("foo").quote
// it says
Uncaught SyntaxError: Got UNFINISHED STRING expected EOF
 at line 1 col 19
exports.quote = """;

But that if you do:

Modules.removeAllCached();
require("Storage").write("foo",`exports.quote = "\\"";`);
require("foo").quote

it works?

But that if you put that extra slash:

exports.quote = "\\"";

into a module file then it doesn't?

@olliephillips
Copy link

olliephillips commented Feb 25, 2019

Simple examples might not suffice. And it may even be a module/use case specific issue. My findings which led me to remove the extra slash from tinyMQTT below:-

With the double escaping, and loaded as normal (not from storage), tinyMQTT appeared to work fine. The problem manifested on the subscription end. It stopped receiving messages after 30 or so seconds (I was publishing at 1 second intervals and received 31 messages on the subscribing client and no more - consistently).

As I recall, there was nothing to indicate a problem on Espruino.

With double escaping removed, publishing was as before but client was able to receive subscribed messages indefinitely. The escaping was added so that the tinyMQTT module could be used from storage, something @MaBecker had been doing without problem as I understand.

So really, what we can say, is that for tinyMQTT there is a an issue somewhere with double escaping, when the module is used normally, which is not an issue when used from storage. I'm inclined to believe the broker has a problem with the payload of the message (not tested), so I'm not sure if this an Espruino problem given the above. Just my thoughts.

@gfwilliams
Copy link
Member

As far as I can tell this is not a bug or even close.

I thought when this was closed I'd explained the issue, but I don't think I had properly, and changes were suggested for tinyMQTT that should never have gone in.

It's literally just an issue with quotes:

print(`\"`)
// Does not output
\"
// It actually just outputs
"

Basically using templated (backtick) Strings to write modules to storage allows you to copy/paste them most of the time, but it's like any other String. If there's an escape character in it then the escape character gets interpreted as that String is parsed. The escape character never actually makes it into flash storage as it should do - you could check just by comparing the filesize of what's in Storage vs the actual module.

The double escaping isn't some magic Espruino feature, it's just what you have to do if you want to include an escaped String inside another string... But if you're not putting a string in a string (eg when the module is a file) then you don't need it.

@MaBecker did you submit any other PRs with double-escaping added? If so I think we should revert them.

@MaBecker
Copy link
Contributor Author

As far as I can tell this is not a bug or even close.

I totally agree!

This is the way I suggest to replace '\x00' with String.fromCharCode(0)

// sample code
var fromCharCode = String.fromCharCode;

quote = fromCharCode(0);
console.log('direct:',E.toArrayBuffer(quote).toString(10));

Modules.removeAllCached();
require("Storage").write("foo","exports.quote = fromCharCode(0);");
console.log('from storage:',E.toArrayBuffer( require("foo").quote).toString(10));

@MaBecker did you submit any other PRs with double-escaping added? If so I think we should revert them.

No because it was not clear if this is the correct solution.

So @gfwilliams decide by your own how to handle '\x00' used in this modules:

DigoleBuf.js, DigoleHW.js, MQTT.js and SBUS.js

@gfwilliams
Copy link
Member

I think the current solution (single quoted \x00) is totally fine.

As with any string at all, you need to escape what goes inside it properly. If I tweaked those modules not to include the \ then it'll just be something else in the future...

Maybe https://www.espruino.com/File+Converter should just be extended so that it'll produce a properly quoted templated String for you to copy and paste so you have something easier when you try to load into Storage?

The real solution here would be to have a file loader in the Web IDE itself though.

@MaBecker
Copy link
Contributor Author

Sorry I can not see a way how to make this work if saving to storage.

@gfwilliams
Copy link
Member

Sorry I can not see a way how to make this work if saving to storage.

This doesn't actually have anything to do with modules, or storage.

What you're saying is "I can't paste the contents of a file into a JS string without modification" - and that is true. There's no way around it.

While it's obvious that you can't just copy/paste a JPEG file into a String, it's less obvious that you can't usually do it with a module either.

In your example of tinyMQTT, just try this:

var s = require('Storage');
s.write('tinyMQTS' , '......put tinyMQTT.min.js code here.........');
print(s.read('tinyMQTS').length);   

The length of the module in Storage won't be the same as the one you intended to upload (even with your tweaked module), because the module has been corrupted.

With your suggested changes you can still make it work, but it's still corrupted in places. You're still trying to execute a module that has the char code 255 in it in a few places, which is far from ideal and may well break in the future.

Let's go for a really simple example:

>print("Hello\nWorld")
Hello
World

That's great. But what if you try and put that into a templated string and execute it?

eval(`print("Hello\nWorld")`)
Uncaught SyntaxError: Got UNFINISHED STRING expected EOF
 at line 1 col 7
print("Hello
      ^

Because print("Hello\nWorld") is 21 chars long, but ``print("Hello\nWorld").length==20 because `\n` was already turned into a newline - and:

print("Hello
World")

isn't valid JS.

But we can't just tell people never to use the \ character in a String in any JS module

How do you fix it?

All you need to do is use https://www.espruino.com/File+Converter to convert the file into a proper JavaScript string by escaping the bits that need escaping.

I just added a new bit down the bottom where it'll create a templated String for you. All it does is replace \ and ${ characters automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants