Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 5575] Patch to fix memory leaks/errors in Lua plugin

[bugzilla-daemon@xxxxxxxxxxxxx]

Peter Wu changed bug 5575

What	Removed	Added
CC		peter@lekensteyn.nl

Comment # 30 on bug 5575 from Peter Wu

As far as I can see:

 - Many Xxx types (Address, Tvb, ...) are just pointers (address *, struct
_wslua_tvb)
 - There are non-pointer types such as typedef gint64 Int64
 - The Xxx values are passed by value (that is, toXxx/checkXxx returns a Xxx
and pushXxx stores a Xxx).

I guess that a pointer to pointer/structures/gint64 is stored as userdata such
that it can be marked as deallocated (not implemented yet? See below).


Now there are basically two types of resources:

 - Structures used only for Lua and not passed to other API functions (Address)
 - Types that have side-effects (registering a protocol/tap: Proto; Listener)

(IOTW, one resource is owned by Lua, the other by C.)

The first one can perfectly be handled by __gc AFAIK:

  static int Address__gc(lua_State *L) {
    Address addr = toAddress(L);
    // if already dellocated, skip
    if (_already_freed(addr)) return 0;
    free_address(addr);
    _mark_as_freed(addr);
    return 0;
  }

(for suitable _already_freed/_mark_as_freed, e.g. checking/setting an "expired"
flag or storing a NULL pointer in the userdata).


The second resource type needs more careful treatment:

 - When __gc is called (normally, via val=nil; or wrongly by val:__gc()), the
structure cannot be reclaimed directly because it might be in use (hfinfo,
dissector registrations, etc.)
 - Such resources can be invalidated from the C code while Lua still has a
reference to this. Solution: use _mark_as_freed as above.
 - Deregistration must be coordinated through the C code (cleanup_routine must
mark all Tvbs are freed (_mark_as_freed) and actually release resources). Like
the current clear_outstanding_Xxx.
 - TODO check if there are deeper dependencies, for example, should fields be
removed before dissectors are deregistered?


I already went through all callers of checkXxx and toXxx and it seems that
there are many of them either have the same effect (FAIL_ON_NULL) or should
behave like FAIL_ON_NULL because of NULL-ptr derefs otherwise. Therefore I
propose:

 - Make toXxx and checkXxx behave more like the other luaL_check* functions.
That is, toXxx returns NULL if a check fails while checkXxx throws.
 - Use toXxx consistently in __gc
 - Use checkXxx consistently elsewhere
 - toXxx will error out if the type is unexpected (e.g. tvb.__gc(pinfo)
 - checkXxx will also error if marked as freed ("expired")
 - isXxx returns FALSE if the type mismatches, but errors if expired
convert all toXxx to just check for the type, returning NULL

The above semantics work for all pointer types and then the FAIL_ON_XXX
parameter can also be removed (removal of FAIL_ON_NULL_MEMBER is already
proposed in https://code.wireshark.org/review/14791).

The special item that will not work with the above is Uint64 because it is not
a pointer. What about storing the uint64 directly in a userdata, bypassing the
existing pushXxx, toXxx, etc.?


With this I hope to improve memory safety and simplify error handling (some
methods silently return nil if the pointer is invalid, others throw an error,
others just crash). Later the Reload Lua plugins functionality can be made more
solid (e.g. not abort on forgotten resource handling like primed fields).

---

      You are receiving this mail because:

• You are watching all bug changes.