random crashes in luajit gc because of memory corruption

[koreader]

random crashes in luajit gc because of memory corruption

[thotypous]

Update: turning on LUA_USE_ASSERT and LUA_USE_APICHECK, I got the following output just before the crash:

koreader-base: lj_gc.c:192: gc_traverse_tab: Assertion `!(((&n->key)->it) == (~0u))' failed.

[hwhw]

Did you check with latest luajit? The update is part of my outstanding PR.
Am 12.08.2013 17:46 schrieb "Paulo Matias" [email protected]:

Update: turning on LUA_USE_ASSERT and LUA_USE_APICHECK, I got the
following output just before the crash:

koreader-base: lj_gc.c:192: gc_traverse_tab: Assertion `!(((&n->key)->it) == (~0u))' failed.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/43#issuecomment-22502821
.

[thotypous]

@hwhw Yes, the backtrace above is using latest luajit (362260b24a7d6f73a2dfe36964441898ebaae8bc), one revision ahead of the one checked out in your outstanding PR, but that should not matter for this issue.

I am currently trying to use valgrind, but perhaps running valgrind in the Kindle is not so easy :P I will try to reproduce it in the emulator, but I am not sure I will be able to do it, because it could be a issue specific to the ARM interpreter or another device-specific thing..

[houqp]

hrm, so does it only happen in filechooser?

[thotypous]

@houqp On my tests, yes, the crash only occurred when using the FileChooser. But I also observed the mentioned memory corruption in the glyphs which might be related. I think there is something causing memory corruption in koreader-base, and I am trying to debug it. The approach I will try now is to hunt and fix all invalid writes/reads on the emulator build using valgrind, and see if that solves the issue. If that does not solve, I still have to think.

[thotypous]

I managed to "fix" it. Perhaps it was really a memory corruption. Found this in the valgrind log when running the emulator. So I disabled the affected lines in paintRoundedCorner (below), then the crashes and the font glyph corruption just stopped!

Now we only need to see how to really fix it, I mean without loosing the rounded corners.

P.S.: Just to be clear, I also tested in the real device.

diff --git a/blitbuffer.c b/blitbuffer.c
index 66bc84d..d4ce8cb 100644
--- a/blitbuffer.c
+++ b/blitbuffer.c
@@ -895,8 +895,8 @@ static int paintRoundedCorner(lua_State *L) {
                }

                for(tmp_y = y; tmp_y > y2; tmp_y--) {
-                       setPixel(dst, (w-r)+off_x+x-1, (h-r)+off_y+tmp_y-1, c);
-                       setPixel(dst, (w-r)+off_x+tmp_y-1, (h-r)+off_y+x-1, c);
+                       //setPixel(dst, (w-r)+off_x+x-1, (h-r)+off_y+tmp_y-1, c);
+                       //setPixel(dst, (w-r)+off_x+tmp_y-1, (h-r)+off_y+x-1, c);

                        setPixel(dst, (w-r)+off_x+tmp_y-1, (r)+off_y-x, c);
                        setPixel(dst, (w-r)+off_x+x-1, (r)+off_y-tmp_y, c);
@@ -904,8 +904,8 @@ static int paintRoundedCorner(lua_State *L) {
                        setPixel(dst, (r)+off_x-x, (r)+off_y-tmp_y, c);
                        setPixel(dst, (r)+off_x-tmp_y, (r)+off_y-x, c);

-                       setPixel(dst, (r)+off_x-tmp_y, (h-r)+off_y+x-1, c);
-                       setPixel(dst, (r)+off_x-x, (h-r)+off_y+tmp_y-1, c);
+                       //setPixel(dst, (r)+off_x-tmp_y, (h-r)+off_y+x-1, c);
+                       //setPixel(dst, (r)+off_x-x, (h-r)+off_y+tmp_y-1, c);
                }
        }
        return 0;

[houqp]

wow, nice catch! looks like there is a problem with y coordinate calculation. I will look into that as well. Do you have a stable way to reproduce the segfault?

[thotypous]

@houqp In my device I can reproduce it by building the currently last commit (1c3bf6f2cb) of my frontlight3 branch with the following patch:

diff --git a/frontend/ui/uimanager.lua b/frontend/ui/uimanager.lua
index 68853b0..c179a80 100644
--- a/frontend/ui/uimanager.lua
+++ b/frontend/ui/uimanager.lua
@@ -1,7 +1,6 @@
 require "ui/geometry"
 require "ui/device"
 require "ui/inputevent"
-require "ui/widget/container"
 require "ui/screen"
 require "debug"
 require "gettext"

Then I open koreader in /mnt/us/documents, enter in some directory, open the main menu of the filechooser, close it, go back to the upper directory, do some random gestures with two fingers. Eventually it crashes, and most of the time I do not need to do all these steps, only need to enter some directory and go back to /mnt/us/documents.

To reproduce the glyph corruption, just open any document and go to the first menu in the upper toolbar (the one which has an option "Table of contents"). The "b" glyph should be corrupted.

[houqp]

Is it only reproducible on kindle? I tried it on emulator but cannot get the segfault.

[thotypous]

@houqp Perhaps yes. I only got the Invalid memory read/write errors and, very later, a crash, when running the emulator inside valgrind. When not using valgrind, the emulator did not crash.

EDIT: I just made this build which crashes in my Kindle PaperWhite (firmware ver 5.3.4) just by calling ./koreader.sh /mnt/us/documents :P https://hal9k.ifsc.usp.br/~matias/floss/koreader/segfault-koreader-v2013.03-414-g44258d4.zip
It was built by using my frontlight3 branch and removing require "ui/widget/container" as I described in a previous comment.

[thotypous]

Hmmm I think the issue might not be in paintRoundedCorner itself. It might not even be in koreader-base as I originally thought. I had noted that paintRoundedCorner was being called to draw something at off_y=783 with h=52, which surpasses the height of my 800x600 emulator. I had put some luaL_error calls in paintRoundedCorner so that I could track where in the Lua code it was being called.

diff --git a/frontend/ui/widget/container.lua b/frontend/ui/widget/container.lua
index c5cdb44..3205ae2 100644
--- a/frontend/ui/widget/container.lua
+++ b/frontend/ui/widget/container.lua
@@ -171,6 +171,8 @@ function FrameContainer:paintTo(bb, x, y)

        --@TODO get rid of margin here?  13.03 2013 (houqp)
        if self.background then
+               print('x=',x, 'y=',y, 'container_width=',container_width, 'container_height=',container_height)
+               --if container_height + y > 800 then container_height = 800 - y end
                bb:paintRoundedRect(x, y, container_width, container_height,
                                                self.background, self.radius)
        end

x=  0   y=  0   container_width=    600 container_height=   835
x=  210 y=  783 container_width=    52  container_height=   52
invalid write: setPixel(0x407d2560, 247, 834, 0) [600, 800]
lua config error: ./frontend/ui/graphics.lua:23: r=15, x=210, y=783, w=52, h=52

stack traceback:
    [C]: in function 'paintRoundedCorner'
    ./frontend/ui/graphics.lua:23: in function 'paintBorder'
    ./frontend/ui/graphics.lua:50: in function 'paintRoundedRect'
    ./frontend/ui/widget/container.lua:176: in function 'paintTo'
    ./frontend/ui/widget/container.lua:294: in function 'paintTo'
    ./frontend/ui/widget/group.lua:36: in function 'paintTo'
    ./frontend/ui/widget/group.lua:96: in function 'paintTo'
    ./frontend/ui/widget/container.lua:186: in function 'paintTo'
    ./frontend/ui/widget/container.lua:294: in function 'paintTo'
    ./frontend/ui/widget/container.lua:294: in function 'paintTo'
    ./frontend/ui/uimanager.lua:202: in function 'run'
    ./reader.lua:279: in main chunk

By uncommenting the if in the patch above, the errors cease... Still tracking down, but apparently the object is a Button, and it is drawn in the first screen without needing to interact with koreader at all (Where would this button be? Which button is that? I don't see any button in the screen when koreader opens...)

[houqp]

does it only happen in filechooser? i will try to reproduce it on my side.

BTW, i have already started writing a real file manager yesterday ;p

[thotypous]

Apparently the out-of-bounds memory write is triggered only when the filechooser is started. If I open koreader passing a document as the argument, it does not occur. I will try with your new file manager to see if it is fixed.

[hwhw]

Arguably, we should clamp coordinates in the render routines to valid
values so this can't happen...

2013/8/14 Paulo Matias [email protected]

Apparently the out-of-bounds memory write is triggered only when the
filechooser is started. If I open koreader passing a document as the
argument, it does not occur. I will try with your new file manager to see
if it is fixed.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/43#issuecomment-22634093
.

[thotypous]

Arguably, we should clamp coordinates in the render routines to valid values so this can't happen...

Or trigger a luaL_error if the rect coordinates are out of the bounds. We already have ASSERT_BLITBUFFER_BOUNDARIES which can be enabled via the DEBUG flag, but checking only the rect coordinates would have much less overhead and maybe it could be enabled in release builds. There are other alternatives also. We could make a simple regression testing framework so it could be easy to run tests from time to time in the emulator (maybe even using valgrind).

[thotypous]

@houqp Dimensions have changed but the out-of-bounds rect is still there with koreader at commit 2e8b5cb39c7042d406a1787d2b84ce32eb9e9073:

x=  0   y=  0   container_width=    610 container_height=   846
x=  5   y=  41  container_width=    600 container_height=   772
x=  215 y=  761 container_width=    52  container_height=   52
invalid write: setPixel(0x40650fe8, 252, 812, 0) [600, 800]
lua config error: ./frontend/ui/graphics.lua:23: r=15, x=215, y=761, w=52, h=52

stack traceback:
    [C]: in function 'paintRoundedCorner'
    ./frontend/ui/graphics.lua:23: in function 'paintBorder'
    ./frontend/ui/graphics.lua:50: in function 'paintRoundedRect'
    ./frontend/ui/widget/container.lua:176: in function 'paintTo'
    ./frontend/ui/widget/container.lua:294: in function 'paintTo'
    ./frontend/ui/widget/group.lua:36: in function 'paintTo'
    ./frontend/ui/widget/group.lua:96: in function 'paintTo'
    ./frontend/ui/widget/container.lua:186: in function 'paintTo'
    ./frontend/ui/widget/container.lua:294: in function 'paintTo'
    ./frontend/ui/widget/group.lua:96: in function 'paintTo'
    ./frontend/ui/widget/container.lua:186: in function 'paintTo'
    ./frontend/ui/widget/container.lua:294: in function 'paintTo'
    ./frontend/ui/uimanager.lua:202: in function 'run'
    ./reader.lua:172: in main chunk

[houqp]

we can probably check out-of-bound in pure lua code and throw an error loudly in debug mode. if an out-of-bound happened, there must a bug in the UI. might be a good thing to detect and clamp coordinates in release, but hey! we haven't had a real release since the first one, lol.

@thotypous , if we have a regression testing framework, that will be super awesome!

ok, I can reproduce the bug now :) will look into it today.

[houqp]

@thotypous could you try my latest PR in koreader?

[thotypous]

@houqp Simply perfect, thank you! Tested on the emulator with valgrind and on the Kindle and the issue is gone.

[houqp]

Thank you so much for helping me trouble shooting the problem! I will never find out the cause without your debug print ;p