ejabberd_zlib_drv: avoid overflow when OOM

[msantos]

No description provided.

[michalwski]

Thanks for the PR @msantos! I have 2 questions:

1. Could you describe briefly when does this help and how is it suppose to behave in case of OOM?
2. See the question in code.

[michalwski]

I'm not sure if abort is the best choice here. How about driver_failure?

[msantos]

1. driver_alloc() is basically a wrapper around the system malloc(). malloc() may return NULL under certain conditions leading to a buffer overflow. This change will crash the VM if this occurs.

2. Use of erl_exit()/erts_exit() depend on the erlang version. abort() is a workaround from the fast_tls driver since mongooseim supports erlang 17, 18 and 19. Upstream I added a compile time check:

processone/ezlib@e7751e5

I can add this check in another PR if wanted.

driver_failure() is reasonable if the system will deal gracefully with having the zlib driver unloaded in a running system. Calling erts_exit() follows what the erlang inet driver does in OOM conditions.

3. Any interest in upstreaming the local patches and using the ezlib driver?

[michalwski]

Thanks for the explanation, now I understand your work better. Did run into the issue on your system? Is it somehow connected to ZLib bomb attack? https://en.wikipedia.org/wiki/Zip_bomb.

[msantos]

Just from reading the code, however a zip bomb would be a good way of triggering it.

…

On Jan 2, 2017 7:42 PM, "Michał Piotrowski" ***@***.***> wrote: Thanks for the explanation, now I understand your work better. Did run into the issue on your system? Is it somehow connected to ZLib bomb attack? https://en.wikipedia.org/wiki/Zip_bomb. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJpa9IZiCL25tKVRgZ0nhwiofUUEK7wks5rOOK0gaJpZM4LVK3J> .

[michalwski]

Regarding the zlib bomb, we are already trying to prevent such an attack. Take a look at the size_limit parameter. It specifies the maximum size of unzipped data. The size is configurable so you can set it to your needs. Do you think this would help?

[msantos]

Yes, that option is a great way of preventing zip bombs. Unfortunately there are many ways to force a system to run out of memory. Checking for memory allocation failure prevents undefined behaviour in case of error.

[michalwski]

Checking for memory allocation failure prevents undefined behaviour in case of error.

Right, I have only one issue with it - stopping the entire VM. Maybe, it would be ok, to just stop the port driver and linked ejabberd_c2s process, so only one user is affected. This should free some memory also.

[msantos]

Assuming the inet or fast_tls drivers don't stop the VM first, the behaviour today is:

• on linux, by default the OOM killer terminates the VM by sending a SIGKILL

• on other systems or if the OOM killer is disabled on linux, anything can happen but probably the VM will segfault

So it may be possible to unload the driver and have a functioning system but the next call into the network may force the VM to abort anyway (and hopefully be restarted by heart).

If we decide to go with driver_failure(), it will be difficult to simulate an OOM condition to test if everything continues to work. One idea might be to add a "fail" command to the driver that simply returns driver_failure() to force the driver to unload during the test run.

[michalwski]

Ok, this sounds reasonable. Thanks for explanations and your contribution.

[michalwski]

One more thing. Could you create another PR regarding the Erlang version check?

Use of erl_exit()/erts_exit() depend on the erlang version. abort() is a workaround from the fast_tls driver since mongooseim supports erlang 17, 18 and 19. Upstream I added a compile time check:
processone/ezlib@e7751e5
I can add this check in another PR if wanted.