Skip to content

Fix out-of-bounds crash in PluginIterator_Next#2445

Open
SCP-076 wants to merge 3 commits intoalliedmodders:masterfrom
SCP-076:fix
Open

Fix out-of-bounds crash in PluginIterator_Next#2445
SCP-076 wants to merge 3 commits intoalliedmodders:masterfrom
SCP-076:fix

Conversation

@SCP-076
Copy link
Copy Markdown

@SCP-076 SCP-076 commented Apr 22, 2026

fixes #1880
Previously, PluginIterator_Next checked MorePlugins() before calling NextPlugin(). This flawed logic caused the native to return true even when NextPlugin() pushed the internal iterator to the end() of the list.

This commit swaps the execution order: the iterator is now advanced first, and then validated. This ensures Next() correctly returns false when reaching the end of the plugin list.

This prevents the native from returning true when the iterator has already reached the end() of the list, which previously led to a crash when accessing the Plugin property.
Copy link
Copy Markdown
Member

@Kenzzer Kenzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, but this is still going to create undefined behaviour/crash if the iterator is advanced again after being exhausted.

Comment thread core/logic/smn_core.cpp
SCP-076 added 2 commits April 22, 2026 22:21
Reflect the new NativeError thrown when calling Next() on an exhausted iterator.
Copy link
Copy Markdown
Member

@Kenzzer Kenzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much needed sanity around the plugin iterator, thanks for the work. Will let this sit for a few days to give the others a chance to comment if they want to.

@SCP-076
Copy link
Copy Markdown
Author

SCP-076 commented Apr 22, 2026

This is much needed sanity around the plugin iterator, thanks for the work. Will let this sit for a few days to give the others a chance to comment if they want to.

Thanks. Just to be clear, the legacy iterator from GetPluginIterator() can actually still cause crashes; this PR strictly focuses on making the long-broken Methodmap version fully usable and safe.

The current mix of the Methodmap implementation and its base class is quite confusing and definitely calls for a proper refactor someday.

BTW, One quick thought: since its traversal logic differs entirely from the legacy one, should we also check m_hasStarted in PluginIterator_Plugin_get and throw an error to enforce that Next() is called at least once? Otherwise, if someone uses a do-while loop for traversal, the first two plugin objects retrieved will be duplicates. I suppose this might be outside the scope of this PR, though.
Actually, never mind. With the current g_PlIter mix, doing this would require either breaking the ABI or using dynamic_cast. It's a dead end for now.

@SCP-076
Copy link
Copy Markdown
Author

SCP-076 commented Apr 23, 2026

To prevent legacy iterator crashes without altering its expected behavior, we could add similar bounds checking to the underlying core IPluginIterator. However, I'm hesitant to touch the core to avoid unintended side effects. A safer compromise is to add the check directly inside the ReadPlugin native:

static cell_t ReadPlugin(IPluginContext *pContext, const cell_t *params)
{
	Handle_t hndl = (Handle_t)params[1];
	HandleError err;
	IPluginIterator *pIter;

	HandleSecurity sec;
	sec.pIdentity = g_pCoreIdent;
	sec.pOwner = pContext->GetIdentity();

	if ((err=handlesys->ReadHandle(hndl, g_PlIter, &sec, (void **)&pIter)) != HandleError_None)
	{
		return pContext->ThrowNativeError("Could not read Handle %x (error %d)", hndl, err);
	}
	
	// Check at the native level without modifying IPluginIterator->GetPlugin()
	if(!pIter->MorePlugins())
		return BAD_HANDLE;
	
	IPlugin *pPlugin = pIter->GetPlugin();
	if (!pPlugin)
	{
		return BAD_HANDLE;
	}

	pIter->NextPlugin();

	return pPlugin->GetMyHandle();
}

Let me know if you'd like me to include this fix to make the legacy iterator completely crash-proof from any SP calls.

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

Successfully merging this pull request may close these issues.

PluginIterator crash for do-while

2 participants