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

ESP32: remove use of luaM_free in file module #2631

Merged
merged 2 commits into from
Feb 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions components/base_nodemcu/include/lnodeaux.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,9 @@ inline bool luaX_valid_ref(lua_ref_t ref) {
return ref > 0;
}

// luaX_pushlstring is the same as lua_pushlstring except it will return nonzero in case of error
// (instead of throwing an exception and never returning) and will put the error message on the top of the stack.
int luaX_pushlstring(lua_State* L, const char* s, size_t l);


#endif
25 changes: 24 additions & 1 deletion components/base_nodemcu/lnodeaux.c
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,30 @@ void luaX_unset_ref(lua_State* L, lua_ref_t* ref) {
// luaX_set_ref pins a reference to a lua object, provided a registry
// or stack position
void luaX_set_ref(lua_State* L, int idx, lua_ref_t* ref) {
luaX_unset_ref(L, ref); // make sure we free previous reference
luaX_unset_ref(L, ref); // make sure we free previous reference
lua_pushvalue(L, idx); // push on the stack the referenced index
*ref = luaL_ref(L, LUA_REGISTRYINDEX); // set the reference (pops 1 value)
}

// pushlstring_t is a helper struct to provide a protected lua_pushlstring call
typedef struct {
const char* s;
size_t l;
} pushlstring_t;

// safe_pushlstring is a private function meant to be called in protected mode.
static int safe_pushlstring(lua_State* L) {
pushlstring_t *ps = (pushlstring_t*)lua_touserdata(L,-1);
lua_pushlstring(L, ps->s, ps->l);
return 1;
}

// luaX_pushlstring is the same as lua_pushlstring except it will return nonzero in case of error
// (instead of throwing an exception and never returning) and will put the error message on the top of the stack.
int luaX_pushlstring(lua_State* L, const char* s, size_t l) {
lua_pushcfunction(L, &safe_pushlstring);
pushlstring_t* ps =(pushlstring_t*) lua_newuserdata(L, sizeof(pushlstring_t));
ps->s = s;
ps->l = l;
return lua_pcall(L, 1, 1, 0);
}
36 changes: 17 additions & 19 deletions components/modules/file.c
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "module.h"
#include "lauxlib.h"
#include "lnodeaux.h"
#include "lmem.h"
#include "platform.h"

Expand Down Expand Up @@ -140,7 +141,6 @@ static int file_obj_free( lua_State *L )
if (ud->fd) {
// close file if it's still open
vfs_close(ud->fd);
ud->fd = 0;
}

return 0;
Expand Down Expand Up @@ -383,32 +383,27 @@ static int file_stat( lua_State* L )
// g_read()
static int file_g_read( lua_State* L, int n, int16_t end_char, int fd )
{
static char *heap_mem = NULL;
// free leftover memory
if (heap_mem) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was added to free memory which was left allocated by a previous call. Standard execution flow doesn't need that, but heap_mem isn't freed in case lua_pushlstring() throws a memory exception as noted in #1541 (comment).

If we keep considering this corner case then bufsize would also need to be declared static.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsaurus I added a new function to make a protected call to lua_pushlstring() and avoid this issue. The new luaX_pushlstring() returns nonzero instead of throwing an exception, allowing you to free memory. This function could be useful elsewhere.

Please check it out and let me know :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent solution!

luaM_free(L, heap_mem);
heap_mem = NULL;
}
char *heap_mem = NULL;

if(n <= 0)
n = FILE_READ_CHUNK;

if(end_char < 0 || end_char >255)
end_char = EOF;


if(!fd)
return luaL_error(L, "open a file first");

char *p;
int i;
size_t bufsize = n;

if (n > LUAL_BUFFERSIZE) {
// get buffer from heap
p = heap_mem = luaM_malloc(L, n);
p = heap_mem = luaM_malloc(L, bufsize);
} else {
// small chunks go onto the stack
p = alloca(n);
p = alloca(bufsize);
}

n = vfs_read(fd, p, n);
Expand All @@ -424,21 +419,24 @@ static int file_g_read( lua_State* L, int n, int16_t end_char, int fd )
i = n;
}

int err = 0;

if (i == 0 || n == VFS_RES_ERR) {
if (heap_mem) {
luaM_free(L, heap_mem);
heap_mem = NULL;
}
lua_pushnil(L);
return 1;
} else {
vfs_lseek(fd, -(n - i), VFS_SEEK_CUR);
err = luaX_pushlstring(L, p, i); // On error it will return nonzero and leave a message on top of the stack.
}

vfs_lseek(fd, -(n - i), VFS_SEEK_CUR);
lua_pushlstring(L, p, i);
if (heap_mem) {
luaM_free(L, heap_mem);
heap_mem = NULL;
luaM_freearray(L, heap_mem, bufsize, char);
}

if (err){
lua_error(L); // luaX_pushlstring failed and the error message is on top of the stack. Throw it.
// never returns
}

return 1;
}

Expand Down