Out-of-memory crash

[zbjornson]

This is a pathological case and it should return null, but this causes a "heap out of memory" error:

Qty.parse("58261da44b642352442b8060")

because this loop attempts to make a string of 64,235,244 repetitions of "4b " (i.e. 192 MB):

      // in var parse = function(val) {
      for (var i = 0; i < Math.abs(n) ; i++) {
        nx += x;
      }

I'm not exactly sure what that loop is supposed to be doing. Should n be clamped to the length of the input, or some hard-coded default?

[rage-shadowman]

What are you expecting here? Is this intended to be a unitless quantity using a hex scalar value?

[zbjornson]

I was expecting it to return null (as it does for other unparseable quantities). This was unsanitized user input.

[rage-shadowman]

I don't think this project was intended to be used with unsanitized user input. For instance, if the user says "32 degF" to "degC" do they mean they want to see "0 C"? If so, then they used the wrong units. They needed to use the zero-kelvin-relative "tempF" and "tempC" units instead. There is just too much ambiguity with user input.

Although it would be nice to make this handle everything intuitively, I think that is too complex a task to be a reasonable goal. If you could at least semi sanitize the input by making sure the count is a number and giving the user a choice of units from a combo box, that would help to reduce issues like this. Plus, you could limit the unit choices to things relevant to your application (force units, volume units, temperatures, whatever you happen to care about).

That said, I am not the maintainer of this project and he may disagree. Also, if you have any ideas that would make the parsing better, I'm sure any reasonably tested pull request would be appreciated.

[zbjornson]

Intended or not, it's a security and stability concern that this library can possibly consume that much memory.

For what it's worth, this case is correctly booted out by the ruby-units library, of which this library was a port:

irb(main):004:0> Unit.new("58261da44b642352442b8060")
ArgumentError: '58261da44b642352442b8060' Unit not recognized

The regexes and parsing logic in this lib are a bit difficult to follow for a newcomer, but I'll see if I can find where this library and ruby-units diverged such that ruby-units handles this case correctly and submit a PR.

[rage-shadowman]

Yes, but I don't see how you can really keep non-sanitized input from doing this. If you picked something valid like "3846758cm^4394768939465994040" would probably still have the same type of issues. Sanitizing the input is still necessary. However, cleaning up the "parse" function would definitely be nice and would make sanity checks easier like enforcing limits on exponents.

[rage-shadowman]

If I had to guess, I'd say that ruby-units realizes that "da" isn't a valid unit and throws it out at that point. How does ruby-units handle "593720475cm^493920750340227"?

[zbjornson]

That does cause a big memory allocation in ruby-units. I'm working on at least a proof of concept PR that disallows unbounded memory growth. No idea if it will work out, but it's at least interesting to try. Sanitizing the user input (at least in our case where we're doing a sort of natural language processing) is relatively tricky and at the moment looks to involve copying at lot of regexes used by this lib.