 |
lua-l archive |
|
|
||
|
Looks good to me,I tested the new code with the test cases with address sanitizer enabled and no UAF detected and there was no crashes.However, I don't know if it's necessary to do the joining if it's already the same upvalue, I mean it's already the same so can we do something like:if(*up1 == *up2) return;so the code becomes:UpVal **up1 = getupvalref(L, fidx1, n1, &f1);UpVal **up2 = getupvalref(L, fidx2, n2, NULL);if(*up1 == *up2) return; //Already joined.luaC_upvdeccount(L, *up1);*up1 = *up2;(*up1)->refcount++;if (upisopen(*up1)) (*up1)->u.open.touched = 1;luaC_upvalbarrier(L, *up1);On Thu, Jan 10, 2019 at 4:29 PM Philippe Verdy <verdy_p@wanadoo.fr> wrote:Le jeu. 10 janv. 2019 à 15:21, Philippe Verdy <verdy_p@wanadoo.fr> a écrit :Do you suggest...Le jeu. 10 janv. 2019 à 14:49, fady osman <fady.mohamed.osman@gmail.com> a écrit :Hi there,I found a heap-user-after-free bug in lua-5.3.5.The function `lua_upvaluejoin` in file lapi.c at line 1287 suffers from a use after free bug when supplied the same function for parameter f1 and f2 and the same upvalue index, additionally the bug happens only when the upvalue is closed, this happens because the `luaC_upvdeccount` function found in file lgc.c at line 678 will decrement the refcount and then free the upvalue if the refcount is zero and if the upvalue is closed.--------------LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,int fidx2, int n2) {LClosure *f1;UpVal **up1 = getupvalref(L, fidx1, n1, &f1);UpVal **up2 = getupvalref(L, fidx2, n2, NULL);... moving this line:luaC_upvdeccount(L, *up1); //Will delete up1... below this:*up1 = *up2; //up1 is up2 because it's the same upvalue and now it's freed.(*up1)->refcount++; //up1 is freed, yet it's used here.if (upisopen(*up1)) (*up1)->u.open.touched = 1; // yet it's also used hereluaC_upvalbarrier(L, *up1); // used here also???...i.e. here ?}This would then be:LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,int fidx2, int n2) {// LClosure *f1; // commented out, not used ???UpVal **up1 = getupvalref(L, fidx1, n1, /*&f1*/NULL); // commented out f1, not used???UpVal **up2 = getupvalref(L, fidx2, n2, NULL);(*up2)->refcount++; //up2 is not freedif (upisopen(*up2)) (*up2)->u.open.touched = 1; // it's also used hereluaC_upvalbarrier(L, *up2); // used here also???luaC_upvdeccount(L, *up1); //Will delete up1*up1 = *up2; // safe now to change it}--