LUA + musl, garbage collection issue?

LUA + musl, garbage collection issue?

[Scott Valentine]

I have been testing OpenWRT builds using musl as the libc on arm and mips targets, and I have run into an issue which appears to be related to lua.
Essentially, when using the luci web interface to upload a file to the device, I am running into a memory exhaustion problem. A 6 MB upload results in nearly 70MB of memory allocation.
On smaller targets (i.e. 64MB RAM), it pretty much just locks up the system until the watchdog timer expires and reboots.
On bigger targets with enough RAM, the process completes and the memory is freed, but I think this is because the lua process exits.

On eglibc and uclibc builds I only see marginal memory increases during the upload, mostly do to the file getting written to tmpfs.

I'm having trouble tracking down where the issue is, but it looks like lua exclusively uses realloc for managing memory. Is this correct?

I noticed that in order to free memory, it basically calls realloc with 0 as the new size. Is this something musl doesn't handle well?

I'm trying a rebuild with a check for n == 0 in musl's realloc function to just free the pointer, and I'll report back.

What is "the right thing to do" to fix this? Should lua not be using realloc to free memory, or should musl handle the case better, if, in fact this is the problem?

Regards,
-Scott V.

Re: LUA + musl, garbage collection issue?

[Rich Felker]

On Sat, Sep 20, 2014 at 04:41:14PM -1000, Scott Valentine wrote:
> I noticed that in order to free memory, it basically calls realloc
> with 0 as the new size. Is this something musl doesn't handle well?
> 
> I'm trying a rebuild with a check for n == 0 in musl's realloc
> function to just free the pointer, and I'll report back.
> 
> What is "the right thing to do" to fix this? Should lua not be using
> realloc to free memory, or should musl handle the case better, if,
> in fact this is the problem?

This is a bug in lua; it's depending on a bug in glibc. POSIX attempts
to allow the glibc behavior (see the text here:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html)
but this allowance is invalid since it conflicts with the requirements
of ISO C (and, as you can see in the above link, "The functionality
described on this reference page is aligned with the ISO C standard.
Any conflict between the requirements described here and the ISO C
standard is unintentional. This volume of POSIX.1-2008 defers to the
ISO C standard.") The relevant ISO C requirement is:

"The realloc function returns a pointer to the new object (which may
have the same value as a pointer to the old object), or a null pointer
if the new object could not be allocated."

In particular, the only way realloc is permitted to return 0 is if the
operation failed, in which case the old pointer is still valid (not
freed).

But even if the glibc behavior weren't conflicting with the C
standard, it's still not valid for an application to assume it, and
it's still undesirable behavior (because it's inconsistent with
glibc's malloc, which returns a non-null, unique pointer for each
malloc(0), and because it makes it impossible for applications to
reliably determine whether the operation succeeded or failed).

The whole situation is a big enough mess that I feel I can safely say
that any application that ever passes 0 as the size argument to
realloc has potentially-serious bugs. So if lua wants to free memory,
it just needs to call free.

Rich

RE: LUA + musl, garbage collection issue?

[Scott Valentine]

[-- Attachment #1: Type: text/plain, Size: 3342 bytes --]



> Date: Sun, 21 Sep 2014 00:38:31 -0400
> From: dalias@libc.org
> To: musl@lists.openwall.com
> Subject: Re: [musl] LUA + musl, garbage collection issue?
> 
> On Sat, Sep 20, 2014 at 04:41:14PM -1000, Scott Valentine wrote:
> > I noticed that in order to free memory, it basically calls realloc
> > with 0 as the new size. Is this something musl doesn't handle well?
> > 
> > I'm trying a rebuild with a check for n == 0 in musl's realloc
> > function to just free the pointer, and I'll report back.
> > 
> > What is "the right thing to do" to fix this? Should lua not be using
> > realloc to free memory, or should musl handle the case better, if,
> > in fact this is the problem?
> 
> This is a bug in lua; it's depending on a bug in glibc. POSIX attempts
> to allow the glibc behavior (see the text here:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html)
> but this allowance is invalid since it conflicts with the requirements
> of ISO C (and, as you can see in the above link, "The functionality
> described on this reference page is aligned with the ISO C standard.
> Any conflict between the requirements described here and the ISO C
> standard is unintentional. This volume of POSIX.1-2008 defers to the
> ISO C standard.") The relevant ISO C requirement is:
> 
> "The realloc function returns a pointer to the new object (which may
> have the same value as a pointer to the old object), or a null pointer
> if the new object could not be allocated."
> 
> In particular, the only way realloc is permitted to return 0 is if the
> operation failed, in which case the old pointer is still valid (not
> freed).
> 
> But even if the glibc behavior weren't conflicting with the C
> standard, it's still not valid for an application to assume it, and
> it's still undesirable behavior (because it's inconsistent with
> glibc's malloc, which returns a non-null, unique pointer for each
> malloc(0), and because it makes it impossible for applications to
> reliably determine whether the operation succeeded or failed).
> 
> The whole situation is a big enough mess that I feel I can safely say
> that any application that ever passes 0 as the size argument to
> realloc has potentially-serious bugs. So if lua wants to free memory,
> it just needs to call free.
> 
> Rich
Interestingly, I added the explicit free and return 0 at the top of musl's realloc function, but I am still seeing the same behavior, so now I am scratching my head. I've eliminated most pieces of the stack as sources of the problem. I can use scp to copy the file without issues, uhttpd pretty much never mallocs anything, I turned off ssl, and luci is almost entire written in lua. It does use the nixio library to actually write the file, but that doesn't really malloc anything and is really just a lightweight lua to native wrapper.
I'm going to try valgrind on the mips target to see who is causing trouble.
Could this possibly have something to do with the read/write stdio functions in musl? I can't really see how, but at present that is the only other implementation specific feature I can find that is relevant. Well, that and musl uses BUFSIZ = 1024 and glibc uses BUFSIZ = 8192. LUA references this variable directly, but it is really only a default.
Thanks,-Scott V. 		 	   		  

[-- Attachment #2: Type: text/html, Size: 3941 bytes --]

Re: LUA + musl, garbage collection issue?

[Justin Cormack]

On Sun, Sep 21, 2014 at 5:38 AM, Rich Felker <dalias@libc.org> wrote:
> On Sat, Sep 20, 2014 at 04:41:14PM -1000, Scott Valentine wrote:
>> I noticed that in order to free memory, it basically calls realloc
>> with 0 as the new size. Is this something musl doesn't handle well?
>>
>> I'm trying a rebuild with a check for n == 0 in musl's realloc
>> function to just free the pointer, and I'll report back.
>>
>> What is "the right thing to do" to fix this? Should lua not be using
>> realloc to free memory, or should musl handle the case better, if,
>> in fact this is the problem?
>
> This is a bug in lua; it's depending on a bug in glibc. POSIX attempts

As pointed out on lua-l but not copied here, Lua is not doing this, it
does call free() in the 0 case, so something else is the issue...

(openwrt does use a patched Lua, might be worth testing with upstream).

Justin

Re: LUA + musl, garbage collection issue?

[Scott Valentine]

On Sunday, September 21, 2014 11:16:13 AM Justin Cormack wrote:
> On Sun, Sep 21, 2014 at 5:38 AM, Rich Felker <dalias@libc.org> wrote:
> > On Sat, Sep 20, 2014 at 04:41:14PM -1000, Scott Valentine wrote:
> >> I noticed that in order to free memory, it basically calls realloc
> >> with 0 as the new size. Is this something musl doesn't handle well?
> >>
> >> I'm trying a rebuild with a check for n == 0 in musl's realloc
> >> function to just free the pointer, and I'll report back.
> >>
> >> What is "the right thing to do" to fix this? Should lua not be using
> >> realloc to free memory, or should musl handle the case better, if,
> >> in fact this is the problem?
> >
> > This is a bug in lua; it's depending on a bug in glibc. POSIX attempts
> 
> As pointed out on lua-l but not copied here, Lua is not doing this, it
> does call free() in the 0 case, so something else is the issue...
> 
> (openwrt does use a patched Lua, might be worth testing with upstream).
> 
> Justin

I see now that it does indeed call free. I was looking at frealloc in the lua source.

In any case, this has been a nasty issue to track down. I have surely traced it to the following code block in luci (by process of elimination):

        local fp
        luci.http.setfilehandler(
                function(meta, chunk, eof)
                        if not fp then
                                if meta and meta.name == "image" then
                                        fp = io.open(image_tmp, "w")
                                else
                                        fp = io.popen(restore_tmp, "w")
                                end
                        end
                        if chunk then
                                fp:write(chunk)
                        end
                        if eof then
                                fp:close()
                        end
                end
        )


Here, "chunk" is a 2048 byte string, and the library calls are to nixio:


static int nixio_file_write(lua_State *L) {
        int fd = nixio__checkfd(L, 1);
        size_t len;
        ssize_t sent;
        const char *data = luaL_checklstring(L, 2, &len);

        if (lua_gettop(L) > 2) {
                int offset = luaL_optint(L, 3, 0);
                if (offset) {
                        if (offset < len) {
                                data += offset;
                                len -= offset;
                        } else {
                                len = 0;
                        }
                }

                unsigned int wlen = luaL_optint(L, 4, len);
                if (wlen < len) {
                        len = wlen;
                }
        }

        do {
                sent = write(fd, data, len);
        } while(sent == -1 && errno == EINTR);
        if (sent >= 0) {
                lua_pushinteger(L, sent);
                return 1;
        } else {
                return nixio__perror(L);
        }
}

So that all looks pretty innocuous... 

Right now, my current work around that actually fixes the issue is to #define BUFSIZ 8192 in musl's stdio.h file.

I have tried increasing LUAL_BUFFERSIZE in luaconf.h (it defaults to BUFSIZ) and reducing the size of "chunk" to as small as 512 bytes, but neither approach fixes the problem.
Also, I don't think LUAL_BUFFERSIZE is relevant to the problem. Any comment?

Either way, it is still unclear as to why the problem occurs, and I can only conjecture because I have no good debugging tools for a musl build.

But basically, I am thinking it partly has to do with the stdio write implementation in musl (i.e. commiting write as soon as the buffer is full).

I'm still not sure where to look to find a proper fix though, I mean essentially it is writing the file to RAM anyway (tmpfs), so the buffer commit should be pretty fast.
Ideas?

When I have time, I'll do a build against eglibc with a reduced BUFSIZ = 1024 (musl's default) and see if the problem is reproduced.

-Scott V.

-- 

Re: LUA + musl, garbage collection issue?

[Szabolcs Nagy]

* Scott Valentine <scottvalen@hotmail.com> [2014-09-23 19:25:47 -1000]:
> 
> In any case, this has been a nasty issue to track down. I have surely traced it to the following code block in luci (by process of elimination):
> 
>         local fp
>         luci.http.setfilehandler(
>                 function(meta, chunk, eof)
>                         if not fp then
>                                 if meta and meta.name == "image" then
>                                         fp = io.open(image_tmp, "w")
>                                 else
>                                         fp = io.popen(restore_tmp, "w")
>                                 end
>                         end
>                         if chunk then
>                                 fp:write(chunk)
>                         end
>                         if eof then
>                                 fp:close()
>                         end
>                 end
>         )
> 
> 
> Here, "chunk" is a 2048 byte string, and the library calls are to nixio:
> 

are you sure the nixio function is called?

if fp is not set then io.open is called which should
use libc fopen, so fp:write should be a wrapper around
fwrite

if the code really calls the nixio function below then
there is no stdio involved, it directly writes to an fd

> static int nixio_file_write(lua_State *L) {
>         int fd = nixio__checkfd(L, 1);
>         size_t len;
>         ssize_t sent;
>         const char *data = luaL_checklstring(L, 2, &len);
> 
>         if (lua_gettop(L) > 2) {
>                 int offset = luaL_optint(L, 3, 0);
>                 if (offset) {
>                         if (offset < len) {
>                                 data += offset;
>                                 len -= offset;
>                         } else {
>                                 len = 0;
>                         }
>                 }
> 
>                 unsigned int wlen = luaL_optint(L, 4, len);
>                 if (wlen < len) {
>                         len = wlen;
>                 }
>         }
> 
>         do {
>                 sent = write(fd, data, len);
>         } while(sent == -1 && errno == EINTR);
>         if (sent >= 0) {
>                 lua_pushinteger(L, sent);
>                 return 1;
>         } else {
>                 return nixio__perror(L);
>         }
> }
> 


> When I have time, I'll do a build against eglibc with a reduced BUFSIZ = 1024 (musl's default) and see if the problem is reproduced.
> 

i think you should try to reproduce the bug with a minimal
test-case where either stdio (io.*) is called or nixio only

Re: LUA + musl, garbage collection issue?

[Scott Valentine]

On Wednesday, September 24, 2014 07:50:01 AM Szabolcs Nagy wrote:
> * Scott Valentine <scottvalen@hotmail.com> [2014-09-23 19:25:47 -1000]:
> > 
> > In any case, this has been a nasty issue to track down. I have surely traced it to the following code block in luci (by process of elimination):
> > 
> >         local fp
> >         luci.http.setfilehandler(
> >                 function(meta, chunk, eof)
> >                         if not fp then
> >                                 if meta and meta.name == "image" then
> >                                         fp = io.open(image_tmp, "w")
> >                                 else
> >                                         fp = io.popen(restore_tmp, "w")
> >                                 end
> >                         end
> >                         if chunk then
> >                                 fp:write(chunk)
> >                         end
> >                         if eof then
> >                                 fp:close()
> >                         end
> >                 end
> >         )
> > 
> > 
> > Here, "chunk" is a 2048 byte string, and the library calls are to nixio:
> > 
> 
> are you sure the nixio function is called?
> 
> if fp is not set then io.open is called which should
> use libc fopen, so fp:write should be a wrapper around
> fwrite

> if the code really calls the nixio function below then
> there is no stdio involved, it directly writes to an fd

You are correct... My brain must be getting tired. 
 
> > static int nixio_file_write(lua_State *L) {
> >         int fd = nixio__checkfd(L, 1);
> >         size_t len;
> >         ssize_t sent;
> >         const char *data = luaL_checklstring(L, 2, &len);
> > 
> >         if (lua_gettop(L) > 2) {
> >                 int offset = luaL_optint(L, 3, 0);
> >                 if (offset) {
> >                         if (offset < len) {
> >                                 data += offset;
> >                                 len -= offset;
> >                         } else {
> >                                 len = 0;
> >                         }
> >                 }
> > 
> >                 unsigned int wlen = luaL_optint(L, 4, len);
> >                 if (wlen < len) {
> >                         len = wlen;
> >                 }
> >         }
> > 
> >         do {
> >                 sent = write(fd, data, len);
> >         } while(sent == -1 && errno == EINTR);
> >         if (sent >= 0) {
> >                 lua_pushinteger(L, sent);
> >                 return 1;
> >         } else {
> >                 return nixio__perror(L);
> >         }
> > }
> > 
> 
> 
> > When I have time, I'll do a build against eglibc with a reduced BUFSIZ = 1024 (musl's default) and see if the problem is reproduced.
> > 
> 
> i think you should try to reproduce the bug with a minimal
> test-case where either stdio (io.*) is called or nixio only

Yes, now that you've pointed out the above, I have something I can work with. I should be able to write a pure lua script to try and reproduce the problem.

-Scott V.

--