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

Add MAX_CONSOLE_LENGTH and COMMAND_MAX_* defines to console.inc #1732

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JoinedSenses
Copy link
Contributor

@JoinedSenses JoinedSenses commented Mar 31, 2022

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

char buffer[1024];

char buffer[1024];

char buffer[1024];

char buffer[1024];

char buffer[1024];

This creates a define to match the underlying SourceMod buffer sizes for Print functions
@Wend4r
Copy link
Contributor

Wend4r commented Mar 31, 2022

I suggest adding defines by

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

from CCommand

@JoinedSenses
Copy link
Contributor Author

JoinedSenses commented Mar 31, 2022

I suggest adding defines by

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

from CCommand

Why is that?

@Wend4r
Copy link
Contributor

Wend4r commented Mar 31, 2022

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" );
		}

@JoinedSenses
Copy link
Contributor Author

JoinedSenses commented Mar 31, 2022

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

@Wend4r
Copy link
Contributor

Wend4r commented Mar 31, 2022

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

image

@JoinedSenses JoinedSenses changed the title Add MAX_CONSOLE_LENGTH define to console.inc Add MAX_CONSOLE_LENGTH and CONSOLE_MAX_* defines to console.inc Mar 31, 2022
@asherkin
Copy link
Member

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.

@Wend4r
Copy link
Contributor

Wend4r commented Mar 31, 2022

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
Copy link
Contributor

Wend4r commented Mar 31, 2022

@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

@JoinedSenses
Copy link
Contributor Author

JoinedSenses commented Mar 31, 2022

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 JoinedSenses changed the title Add MAX_CONSOLE_LENGTH and CONSOLE_MAX_* defines to console.inc Add MAX_CONSOLE_LENGTH and COMMAND_MAX_* defines to console.inc Apr 11, 2022
@Headline
Copy link
Member

Headline commented May 3, 2022

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
Copy link
Member

Headline commented May 4, 2022

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

@asherkin
Copy link
Member

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.

@Headline
Copy link
Member

Headline commented May 25, 2022

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

@peace-maker
Copy link
Member

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

@KyleSanderson
Copy link
Member

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?

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.

6 participants