search

arcapos / luapgsql

Lua binding for PostgreSQL
109 stars 24 forks source link

OOM handling around get_sql_params #10

Closed daurnimator closed 9 years ago

daurnimator commented 9 years ago

Looking at the code in and around get_sql_params, there does not seem to be much checking for calloc, strdup, etc failing.

Most of the calloc callsites can probably be safely replaced with calls to lua_newuserdata, so that you get OOM handling for free from lua (it will longjmp out on failure).

Others such as paramValues[n] = strdup(lua_tostring(L, t)); will need to be fixed on a case by case basis.

mbalmer commented 9 years ago

I a single strdup() fails, the ship will most probably sink anyways... But you are right, I will make a sweep through the code to check for OOM conditions (to prevent that unwanted data ends up in the database).

daurnimator commented 9 years ago

a single strdup() fails, the ship will most probably sink anyways

IMO it's not hard to imagine someone using some gigantic string param (hundreds of megs), and not having enough memory to have two copies around?

mbalmer commented 9 years ago

All memory allocations are now safe. When an OOM condition is there, it will call luaL_error(L, "out of memory") and free all previously allocated memory. I did not go for lua_newuserdata() since that manipulates the stack that I am currently traversing. Further I used lua_tolstring insetad of lua_tostring/strlen where applicable. Will commit after a few more tests.

mbalmer commented 9 years ago

This now committed in the master branch (not yet part of a release).

daurnimator commented 9 years ago

Just finished reviewing; seems good to me :+1:

daurnimator commented 9 years ago

Saw a potential issue: https://github.com/mbalmer/luapgsql/blob/master/luapgsql.c#L780 If allocating paramValues fails, you still iterate over it freeing members. This will segfault.

mbalmer commented 9 years ago

Am 18.02.15 um 17:56 schrieb daurnimator:

Saw a potential issue: https://github.com/mbalmer/luapgsql/blob/master/luapgsql.c#L780 If allocating |paramValues| fails, you still iterate over it freeing members. This will segfault.

I fixed that, thanks!

daurnimator commented 9 years ago

I fixed that, thanks!

Thanks for the quick turnaround.