New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

double-free in CEmuopl::~CEmuopl() #67

Closed
leoaccount opened this Issue Sep 4, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@leoaccount

leoaccount commented Sep 4, 2018

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().

@binarymaster binarymaster added the bug label Sep 26, 2018

@leoaccount

This comment has been minimized.

Show comment
Hide comment
@leoaccount

leoaccount Oct 1, 2018

CVE assigned: CVE-2018-17825

leoaccount commented Oct 1, 2018

CVE assigned: CVE-2018-17825

@Malvineous

This comment has been minimized.

Show comment
Hide comment
@Malvineous

Malvineous Oct 1, 2018

Member

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.

Member

Malvineous commented Oct 1, 2018

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.

@binarymaster

This comment has been minimized.

Show comment
Hide comment
@binarymaster

binarymaster Oct 3, 2018

Member

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();
}
Member

binarymaster commented Oct 3, 2018

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();
}
@Malvineous

This comment has been minimized.

Show comment
Hide comment
@Malvineous

Malvineous Oct 3, 2018

Member

@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?

Member

Malvineous commented Oct 3, 2018

@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?

@leoaccount

This comment has been minimized.

Show comment
Hide comment
@leoaccount

leoaccount Oct 3, 2018

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.

leoaccount commented Oct 3, 2018

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.

@binarymaster

This comment has been minimized.

Show comment
Hide comment
@binarymaster

binarymaster Oct 3, 2018

Member

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.

Member

binarymaster commented Oct 3, 2018

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.

@fgeek

This comment has been minimized.

Show comment
Hide comment
@fgeek

fgeek Oct 4, 2018

CVE-2018-17825 has been assigned for this issue.

fgeek commented Oct 4, 2018

CVE-2018-17825 has been assigned for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment