ffi/koptcontext: fix language handling

[benoit-pierre]

Don't set context field from a string that could be garbage collected…

---

This change is 

[benoit-pierre]

This is enough to fix this flaky test:

koreader-base/spec/unit/koptcontext_spec.lua

Line 203 in e2778f3

assert.are.same(kc_table.language, new_kc_table.language)

(But valgrind still complains about a number of uninitialized memory tests).

[NiLuJe]

That would imply that ffi.new("char[?]", luastring) doesn't actually create a copy but just a pointer to the string data?

That's... weird o_O.

[benoit-pierre]

No, it's a copy alright, but which can be garbage collected too, no?

[NiLuJe]

Right, yeah; but it shouldn't be an issue in theory, I think? (at least outside of busted wrecking havoc on LuaJIT/FFI ;p).

(IIRC, it's also similar to what @ezdiy settled on in his original mupdf bump branch, FWIW).

[benoit-pierre]

It is if the pointer is to be longer lived than the LUA FFI object.

[NiLuJe]

I mean, unless something's killing the ref via a self.language = nil somewhere, the very fact that's it's inside that object pins it to that object lifecycle at worst.

[benoit-pierre]

FWIU the assignment to the context language field does not create a reference to the FFI object.

[NiLuJe]

I'd understand this if we were doing something like self.language = ffi.new("const char*", luastring) (I'm noteven sure if this actually works, can't check rn), because at that point, yes, we're creating a simple cdata pointer to actual Lua data that might get GC'ed before that cdata object itself (and, in the context above, will definitely be collectable as soon as the function returns); but with self.language = ffi.new("char[?]", luastring, #luastring + 1), my understanding is that it's a proper cdata byte array, with a null-terminated copy of the string inside, tracked by the GC; so, as long as any refs to it are alive, it's not released. And self.language should be the only ref to it, so as long as it's not replaced (or self itself is replaced/destroyed), the actual memory should be pinned.

(I don't have anything against the fix, I'd just like to understand what's happening ^^).

[benoit-pierre]

From luajit's documentation:

Garbage Collection of cdata Objects

All explicitly (ffi.new(), ffi.cast() etc.) or implicitly (accessors) created cdata objects are garbage collected. You need to ensure to retain valid references to cdata objects somewhere on a Lua stack, an upvalue or in a Lua table while they are still in use. Once the last reference to a cdata object is gone, the garbage collector will automatically free the memory used by it (at the end of the next GC cycle).

Please note, that pointers themselves are cdata objects, however they are not followed by the garbage collector. So e.g. if you assign a cdata array to a pointer, you must keep the cdata object holding the array alive as long as the pointer is still in use:

[NiLuJe]

Yeah, but, despite C arrays decaying to pointers, this isn't a pointer, it's an array ;).

[benoit-pierre]

The koptcontext field is a pointer, the code is setting a FFI structure member.

[NiLuJe]

Oh, dammit.

koreader-base/ffi/koptcontext_h.lua

Line 142 in e2778f3

char *language;

Yup, never even thought to double-check that.

[NiLuJe]

I keep forgetting that this thing is a struct, this isn't the first time it's tripped me up >_<".

(FWIW, all that I wrote above was based on the false assumption that self was a plain Lua table).

[benoit-pierre]

And those 2 patches take care of the (many) uninitialized memory access Valgrind complains about:

• leptonica (similar fix in upstream's master):

diff --git a/src/pix2.c b/src/pix2.c
index 6c1b06f4..e5b3a9a4 100644
--- a/src/pix2.c
+++ b/src/pix2.c
@@ -1838,7 +1838,7 @@ PIX     *pixd;
     pixGetDimensions(pixs, &ws, &hs, &d);
     wd = ws + left + right;
     hd = hs + top + bot;
-    if ((pixd = pixCreateNoInit(wd, hd, d)) == NULL)
+    if ((pixd = pixCreate(wd, hd, d)) == NULL)
         return (PIX *)ERROR_PTR("pixd not made", procName, NULL);
     pixCopyResolution(pixd, pixs);
     pixCopyColormap(pixd, pixs);
@@ -1917,7 +1917,7 @@ PIX     *pixd;
         return (PIX *)ERROR_PTR("width must be > 0", procName, NULL);
     if (hd <= 0)
         return (PIX *)ERROR_PTR("height must be > 0", procName, NULL);
-    if ((pixd = pixCreateNoInit(wd, hd, d)) == NULL)
+    if ((pixd = pixCreate(wd, hd, d)) == NULL)
         return (PIX *)ERROR_PTR("pixd not made", procName, NULL);
     pixCopyResolution(pixd, pixs);
     pixCopySpp(pixd, pixs);

• libk2pdfopt:

diff --git a/k2pdfoptlib/k2proc.c b/k2pdfoptlib/k2proc.c
index 9052cd68..36f13e89 100644
--- a/k2pdfoptlib/k2proc.c
+++ b/k2pdfoptlib/k2proc.c
@@ -1579,6 +1579,7 @@ printf("Creating single wrmap.\n");
 #endif
             wrmaps0=&_wrmaps0;
             wrectmaps_init(wrmaps0);
+            memset(&wrmap, 0, sizeof (wrmap));
             wrmap.srcpageno=newregion->pageno;
             wrmap.srcwidth=newregion->bmp->width;
             wrmap.srcheight=newregion->bmp->height;
diff --git a/k2pdfoptlib/textrows.c b/k2pdfoptlib/textrows.c
index 88dcf712..2155fcce 100644
--- a/k2pdfoptlib/textrows.c
+++ b/k2pdfoptlib/textrows.c
@@ -529,6 +529,7 @@ printf("    row[%2d].lc/h5050/cap = %3d %3d %3d\n",i,textrows->textrow[i].lcheig
         {
         int lch,rh,c1new,partial,r0,nrt;
         int *prowthresh,*rthresh;
+        prowthresh=NULL;
 
         lch=0;
         if (i>0 && textrows->textrow[i].capheight > textrows->textrow[i-1].capheight*1.8)
diff --git a/k2pdfoptlib/wrapbmp.c b/k2pdfoptlib/wrapbmp.c
index de01921a..adb754c0 100644
--- a/k2pdfoptlib/wrapbmp.c
+++ b/k2pdfoptlib/wrapbmp.c
@@ -235,6 +235,7 @@ exit(10);
         WRECTMAP _wrmap,*wrmap;
 
         wrmap=&_wrmap;
+        memset(wrmap, 0, sizeof (WRECTMAP));
         wrmap->srcpageno = region->pageno;
         wrmap->srcwidth = region->bmp8->width;
         wrmap->srcheight = region->bmp8->height;
@@ -318,6 +319,7 @@ k2printf("3.  wbh=%d x %d, tmp=%d x %d x %d, new_base=%d, wbbase=%d\n",wrapbmp->
     WRECTMAP _wrmap,*wrmap;
 
     wrmap=&_wrmap;
+    memset(wrmap, 0, sizeof (WRECTMAP));
     wrmap->srcpageno = region->pageno;
     wrmap->srcwidth = region->bmp8->width;
     wrmap->srcheight = region->bmp8->height;

[NiLuJe]

this isn't the first time it's tripped me up >_<".

Which is probably where that fixme above it came from ^^. Which also may not entirely be well thought-through in terms of scoping ;p.

[benoit-pierre]

this isn't the first time it's tripped me up >_<".

Which is probably where that fixme above it came from ^^. Which also may not be entirely be well thought-through in terms of scoping ;p.

Right, should I get rid of it?

[NiLuJe]

Right, should I get rid of it?

Yup, and I would definitely add a reminder that this is a struct to explain the roundabout strdup, as I or others will surely forget that fact again in the future ;o).

[benoit-pierre]

    if language then
        -- As `self` is a struct, and `self.language` a `char *`,
        -- we need to make a C copy of the language string.
        self.language = C.strdup(language)
    end

?

[NiLuJe]

Yup, perfect!

[Frenzie]

I keep forgetting that this thing is a struct, this isn't the first time it's tripped me up >_<".

Same. :-)