Skip to content
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

Crash in MicroProfileCreateThreadLog() when (S.nNumLogs > MICROPROFILE_MAX_THREADS) #60

Open
MicrosoftComments opened this issue May 14, 2020 · 0 comments

Comments

@MicrosoftComments
Copy link

In this code, it's perfectly possible for pOldest to still be nullptr when it comes to the end (Which I know from observation).

While this condition has an ASSERT, that doesn't stop it immediately trying to reset the null pointer if you're in a release build, with hilarious consequences.

Easy repro case is to set MICROPROFILE_MAX_THREADS to 1 or 2.

This is a pain if you have a variable number of threads being profiled, so there is no safe level to set MICROPROFILE_MAX_THREADS to.

MicroProfileCreateThreadLog()
...
	if(S.nNumLogs == MICROPROFILE_MAX_THREADS && S.nFreeListHead == -1)
	{
		uprintf("recycling thread logs\n");
		//reuse the oldest.
		MicroProfileThreadLog* pOldest = 0;
		uint32_t nIdleFrames = 0;
		for(uint32_t i = 0; i < MICROPROFILE_MAX_THREADS; ++i)
		{
			MicroProfileThreadLog* pLog = S.Pool[i];
			uprintf("tlactive %p, %d.  idle:%d\n", pLog, pLog->nActive, pLog->nIdleFrames); 
			if(pLog->nActive == 2)
			{
				if(pLog->nIdleFrames >= nIdleFrames)
				{
					nIdleFrames = pLog->nIdleFrames;
					pOldest = pLog;
				}
			}
		}
		MP_ASSERT(pOldest);
		MicroProfileLogReset(pOldest);
	}

Possible fix suggestion is to simply fail the function and return nullptr from MicroProfileCreateThreadLog() if it hasn't got any more room. Although this does rather push the problem to the caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant