sourcemod icon indicating copy to clipboard operation
sourcemod copied to clipboard

Add MAX_CONSOLE_LENGTH and COMMAND_MAX_* defines to console.inc

Open JoinedSenses opened this issue 2 years ago • 15 comments

This creates a define to match the underlying SourceMod buffer sizes for console related methods.

https://github.com/alliedmodders/sourcemod/blob/237db0504c7a59e394828446af3e8ca3d53ef647/core/logic/smn_console.cpp#L91 https://github.com/alliedmodders/sourcemod/blob/237db0504c7a59e394828446af3e8ca3d53ef647/core/logic/smn_console.cpp#L130 https://github.com/alliedmodders/sourcemod/blob/237db0504c7a59e394828446af3e8ca3d53ef647/core/logic/smn_console.cpp#L155 https://github.com/alliedmodders/sourcemod/blob/237db0504c7a59e394828446af3e8ca3d53ef647/core/logic/smn_console.cpp#L177 https://github.com/alliedmodders/sourcemod/blob/237db0504c7a59e394828446af3e8ca3d53ef647/core/logic/smn_console.cpp#L266

JoinedSenses avatar Mar 31 '22 02:03 JoinedSenses

I suggest adding defines by

	enum
	{
		COMMAND_MAX_ARGC = 64,
		COMMAND_MAX_LENGTH = 512,
	};

from CCommand

Wend4r avatar Mar 31 '22 10:03 Wend4r

I suggest adding defines by

	enum
	{
		COMMAND_MAX_ARGC = 64,
		COMMAND_MAX_LENGTH = 512,
	};

from CCommand

Why is that?

JoinedSenses avatar Mar 31 '22 11:03 JoinedSenses

Why is that?

Сommand will not executed over the limit CCommand::Tokenize() (tier1/convar.cpp):

	int nLen = Q_strlen( pCommand );
	if ( nLen >= COMMAND_MAX_LENGTH - 1 )
	{
		Warning( "CCommand::Tokenize: Encountered command which overflows the tokenizer buffer.. Skipping!\n" );
		return false;
	}
		if( m_nArgc >= COMMAND_MAX_ARGC )
		{
			Warning( "CCommand::Tokenize: Encountered command which overflows the argument buffer.. Clamped!\n" );
		}

Wend4r avatar Mar 31 '22 11:03 Wend4r

I see they are part of convar.h, are the values the same for all games?

JoinedSenses avatar Mar 31 '22 11:03 JoinedSenses

I see they are part of convar.h, are the values the same for all games?

image

Wend4r avatar Mar 31 '22 11:03 Wend4r

How do you envision these defines being used by plugins? Are there many plugins out there defining these values themselves?

I believe the 1024 byte formatting buffer is completely arbitrarily sized on SM's side, and is meant to just be "bigger than any plugin could reasonably want to print on a single line" - there is no relation between these functions.

asherkin avatar Mar 31 '22 11:03 asherkin

How do you envision these defines being used by plugins? Are there many plugins out there defining these values themselves?

My case

	bool ExecuteCommands()
	{
		bool bResult = this.hExecuteCommands != null;

		if(bResult)
		{
			int iLength = this.hExecuteCommands.Length;

			char sCommand[COMMAND_MAX_LENGTH]; // instead of the past #define MAX_COMMAND_LENGTH 512

			for(int i = 0; i != iLength; i++)
			{
				this.hExecuteCommands.GetString(i, sCommand, sizeof(sCommand));

			#if defined DEBUG
				LogMessage("\"%s\"", sCommand);
			#endif

				InsertServerCommand("%s", sCommand);
			}

			if(iLength)
			{
				ServerExecute();
			}
		}

		return bResult;
	}

Wend4r avatar Mar 31 '22 11:03 Wend4r

@asherkin, I am of the opinion that if 512 bytes are not enough for the plugin, then let it be written not completely for the further command segment and output error from the handler, than will be incomprehensible

CCommand::Tokenize: Encountered command which overflows the tokenizer buffer.. Skipping

Wend4r avatar Mar 31 '22 11:03 Wend4r

My use case in MAX_CONSOLE_LENGTH is for printing out large text chunks to the client/server console. I had to dig through SM code to figure out the max it allowed, which I strongly felt should have been defined somewhere.

JoinedSenses avatar Mar 31 '22 12:03 JoinedSenses

My only real gripe with this is that if we are going to be defining a maximum console length, we should have this be reflected in sm core as well. I don't like the idea of having constants exist in core that must remain identical to an SP counterpart without any kind of reference.

Headline avatar May 03 '22 09:05 Headline

@asherkin I think having some limit hidden in sm core with no reference for plugins isn't great, however arbitrary the actual size may be. Any information that requires a dive into sm core to figure out is probably something we should clarify for plugins.

Headline avatar May 04 '22 02:05 Headline

I think having some limit hidden in sm core with no reference for plugins isn't great, however arbitrary the actual size may be.

All the formatting that core does should probably be changed (where there isn't an actual practical and obvious limit) to use the fixed-size buffer initially, but if the limit is exceeded allocate a correctly sized buffer and use that for a 2nd attempt. It's either that or we just document it per-function, but unifying these together does not seem like the right approach.

asherkin avatar May 23 '22 15:05 asherkin

I think having some limit hidden in sm core with no reference for plugins isn't great, however arbitrary the actual size may be.

All the formatting that core does should probably be changed (where there isn't an actual practical and obvious limit) to use the fixed-size buffer initially, but if the limit is exceeded allocate a correctly sized buffer and use that for a 2nd attempt. It's either that or we just document it per-function, but unifying these together does not seem like the right approach.

Here's a rough example I'm thinking of, probably along the lines of what you're suggesting?

std::unique_ptr<char[]> dynamicFormat(void *pContext, const int *params, unsigned int param) {
	
	size_t try_size = 1024;
	auto str = std::make_unique<char[]>(try_size);

	size_t len; // bytes written not including null term
	while (true) {
		DetectExceptions eh(pContext);
		len = g_pSM->FormatString(str.get(), try_size - 2, pContext, params, param);
		if (eh.HasException())
			return 0;
		if (len < try_size-1) {
			break;
		} 
		try_size *= 1.5;
		str = std::make_unique<char[]>(try_size);
	}

	return str;
}

Not sure how we'd know what the size would be by the second try

Headline avatar May 25 '22 06:05 Headline

The first try should be on a stack buffer to avoid the heap allocation by default.

peace-maker avatar May 25 '22 14:05 peace-maker

The first try should be on a stack buffer to avoid the heap allocation by default.

indeed. allocations like this will murder throughput. @Headline are you OK to take this on?

KyleSanderson avatar Mar 30 '23 05:03 KyleSanderson