CVE-2018-17825: double-free in CEmuopl::~CEmuopl() · Issue #67 · adplug/adplug

There are several suspected double-free bugs (http://cwe.mitre.org/data/definitions/415.html) with CEmuopl::~CEmuopl() in emuopl.cpp. This destructor calls OPLDestroy(opl[0]) and OPLDestroy(opl[1]) in succession. As defined in fmopl.c, however, OPLDestroy(FM_OPL *OPL) calls OPL_UnLockTable(void), which invokes OPLCloseTable(void). OPLCloseTable(void) frees the four global pointers TL_TABLE, SIN_TABLE, AMS_TABLE, VIB_TABLE by simply using the free() method. Therefore, the two successive calls of OPLDestroy() in CEmuopl::~CEmuopl() are likely to cause double-frees of the four pointers.
One possible fix could be directly assigning each pointer to NULL after calling free().

Sorry meant to reply to this sooner but haven’t had a chance. Yes these look like legitimate bugs, I’m not sure why fmopl needs global state at all but we should really fix that. Assigning them to NULL after the free may fix the double-free error but technically the bug would remain, as creating two OPLs then destroying one will incorrectly cause the second one to stop functioning properly too.

If I remember this correctly it only affects the surround synth (and dueling synths in the Winamp plugin) as these are the only two that use multiple OPLs. So the easiest short-term mitigation if you’re playing untrusted music is to listen in mono or stereo only.

Wouldn’t num_lock global variable protect against double free in this case?

Looks like it’s used exactly to avoid double alloc and double free.

/* lock level of common table */ static int num_lock = 0;

/* lock/unlock for common table */ static int OPL_LockTable(void) { num_lock++; if(num_lock>1) return 0; /* first time */ cur_chip = NULL; /* allocate total level table (128kb space) */ if( !OPLOpenTable() ) { num_lock–; return -1; } return 0; }

static void OPL_UnLockTable(void) { if(num_lock) num_lock–; if(num_lock) return; /* last time */ cur_chip = NULL; OPLCloseTable(); }

@binarymaster Thanks for looking into this. Yes you’re right, this should correctly protect the shared global constants so there should be no problem having multiple instances.

@leoaccount Can you give us more detail about where you are seeing the double-free happen?

Yes. When creating each opl instance, OPL_LockTable() is called and the num_lock is added.

For the creation of opl[0], the num_lock can remain 0 if it happens that OPLOpenTable() returns 0 (see the method OPL_LockTable()). After opl[1] being created successfully, num_lock now becomes 1.

Destroying opl[0] and opl[1] results in two successive calls of OPL_UnLockTable(). Since num_lock=1, both calls will eventually invoke OPLCloseTable() and cause double-free.

For the creation of opl[0], the num_lock can remain 0

Yes, num_lock can remain 0 only in case OPLOpenTable() failed. If it fails, it does not allocate anything and returns -1, and in this case OPLCreate also fails, returning NULL.

OPLDestroy does not check whether OPL argument is NULL. So, if I’m not mistaken, fixing this check should fix the actual bug.