colis-anr / morbig

A static parser for POSIX Shell
Other
190 stars 8 forks source link

Stack overflow in JSON export on large files full of simple commands #82

Closed Niols closed 1 year ago

Niols commented 5 years ago

To reproduce:

for i in $(seq 30000); do echo foo; done > Stack\ overflow.sh
time morbig Stack\ overflow.sh

This will create a file with 30k lines of simple commands foo, on which Morbig takes quite some time to run (hence the time command) and ends up reaching a stack overflow.

On my computer, the limit is somewhere near 23k lines and Morbig takes about half a minute before crashing.

Niols commented 5 years ago

Hum, this seems to be related to the JSON. With morbig --as simple, this takes much longer (I have to go to 60k lines before reaching the Stack Overflow). With morbig --as bin, it doesn't fail (I could try with much larger scripts but it's starting to take a long time).

I'm not sure if there's much we can do here. Maybe document this?

yurug commented 5 years ago

I cannot reproduce. Can you?

Niols commented 5 years ago

Yes I can, on the same computer. Maybe we have different stack sizes?

Niols commented 5 years ago

Hum, no, even when creating a switch from scratch, OPAM goes for Yojson 1.5.0

yurug commented 5 years ago

I successfully reproduce the bug. Thanks.

Niols commented 5 years ago

But this is, I believe, something in Yojson. I don't think that we can do much about it.

Le 28 mars 2019 10:19:31 GMT+01:00, "Yann Régis Gianas" notifications@github.com a écrit :

I successfully reproduce the bug. Thanks.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/colis-anr/morbig/issues/82#issuecomment-477513216

yurug commented 5 years ago

This seems indeed related to the following (fixed?) issue: https://github.com/ocaml-community/yojson/issues/47

Niols commented 5 years ago

So it would seem. The problem lies then in our dependencies that force Yojson to remain in 1.5.0.

Le 28 mars 2019 10:25:11 GMT+01:00, "Yann Régis Gianas" notifications@github.com a écrit :

This seems indeed related to the following (fixed?) issue: https://github.com/ocaml-community/yojson/issues/47

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/colis-anr/morbig/issues/82#issuecomment-477515214

yurug commented 5 years ago

This is due to ppx_deriving_yojson.

yurug commented 5 years ago

I will try to move to deriving-json.

Niols commented 5 years ago

Yet an other deriver? Otherwise, I used ppx_protocol_conv for some time. It is also what we use in CoLiS-Language for Yaml.

Le 28 mars 2019 10:40:31 GMT+01:00, "Yann Régis Gianas" notifications@github.com a écrit :

I will try to move to deriving-json.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/colis-anr/morbig/issues/82#issuecomment-477519944

yurug commented 5 years ago

My first try of using deriving-json is a failure: I get a syntax error, probably triggered by the use of ppx derivers from visitors.

If you know how to move to ppx_protocol_conv, can you try?

Niols commented 5 years ago

I know how to do this, I use it quite often. However, an other way would simply be to ask ppx_deriving_yojson to update their dependencies.

Niols commented 5 years ago

In fact, ppx_deriving_yojson fixed its dependencies 2 months ago. But the new OPAM file isn't in the official repositories. I'll try with it and see if the issue is fixed and I'll open an issue in ppx_deriving_yojson.

Niols commented 5 years ago

OK, I can still reproduce with Yojson 1.7.0. But this isn't so surprising. The problem here is not that we are using List.map on large lists. Our trees aren't large at all. However, on such an input full of simple commands, the tree goes really deep, deep enough to cause a stack overflow in a function that walks through the whole tree.

This would require a consequent change in Yojson, I don't think that they want to do it. There are other parsers and printers for JSON. I know at least Jsonm, but in my experience it's not as good as Yojson, and I already encountered Stack Overflow issues with it that were solved by a switch to Yojson.

yurug commented 5 years ago

OK, let us wait for upstream bug fix.

Niols commented 1 year ago

I cannot reproduce anymore with Yojson 2, even with 300,000 lines. I will close this issue. Feel free to reopen if you still have the problem.