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

Error messages update #13

Merged
merged 10 commits into from
Oct 29, 2023
Merged

Error messages update #13

merged 10 commits into from
Oct 29, 2023

Conversation

MiranDMC
Copy link
Collaborator

Text argument type names in error messages
Added pointer minimal value validation when reading or writing string params.

@MiranDMC MiranDMC requested a review from x87 October 28, 2023 02:44
source/CCustomOpcodeSystem.cpp Outdated Show resolved Hide resolved
cleo_sdk/CLEO.h Outdated Show resolved Hide resolved
Comment on lines 664 to 685
size_t length = bufSize ? bufSize - 1 : 0; // minus terminator char

if (opcodeParams[0].dwParam == 0)
{
length = 0;
LOG_WARNING("Reading string from null pointer in script %s", ((CCustomScript*)thread)->GetInfoStr().c_str());
const char* eStr = "(null)";
length = min(length, strlen(eStr));
if (length) strncpy(buf, eStr, length);
}
else if(opcodeParams[0].dwParam <= CCustomOpcodeSystem::MinValidAddress)
{
LOG_WARNING("Reading string from argument with invalid pointer value (0x%X) in script %s", opcodeParams[0].dwParam, ((CCustomScript*)thread)->GetInfoStr().c_str());
const char* eStr = "(badPtr)";
length = min(length, strlen(eStr));
if (length) strncpy(buf, eStr, length);
return nullptr; // error
}

if(bufSize > 0)
length = min(length, bufSize - 1); // minus terminator char
else
length = 0; // no target buffer
{
char* str = opcodeParams[0].pcParam;
length = min(length, strlen(str));
if (length) strncpy(buf, str, length);
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
size_t length = bufSize ? bufSize - 1 : 0; // minus terminator char
if (opcodeParams[0].dwParam == 0)
{
length = 0;
LOG_WARNING("Reading string from null pointer in script %s", ((CCustomScript*)thread)->GetInfoStr().c_str());
const char* eStr = "(null)";
length = min(length, strlen(eStr));
if (length) strncpy(buf, eStr, length);
}
else if(opcodeParams[0].dwParam <= CCustomOpcodeSystem::MinValidAddress)
{
LOG_WARNING("Reading string from argument with invalid pointer value (0x%X) in script %s", opcodeParams[0].dwParam, ((CCustomScript*)thread)->GetInfoStr().c_str());
const char* eStr = "(badPtr)";
length = min(length, strlen(eStr));
if (length) strncpy(buf, eStr, length);
return nullptr; // error
}
if(bufSize > 0)
length = min(length, bufSize - 1); // minus terminator char
else
length = 0; // no target buffer
{
char* str = opcodeParams[0].pcParam;
length = min(length, strlen(str));
if (length) strncpy(buf, str, length);
}
if(opcodeParams[0].dwParam <= CCustomOpcodeSystem::MinValidAddress)
{
return nullptr; // error
}
else
{
char* str = opcodeParams[0].pcParam;
size_t length = min(bufSize ? bufSize - 1 : 0, strlen(str));
if (length) strncpy(buf, str, length);
}

Copy link
Collaborator Author

@MiranDMC MiranDMC Oct 29, 2023

Choose a reason for hiding this comment

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

I added read string from nullptr as error in previous version, and after some using it I decided it is not what I like. It is common case when you can have text pointer or null returned from somewhere, and not necessary you always care what it is, just use it to print. Old '(null)' returning behaviour was fine after all, but printing ptr like 0x5 is of course some logic error.

I would like to keep "Reading string from argument with invalid pointer value (x)" error message too.

Copy link

@x87 x87 Oct 29, 2023

Choose a reason for hiding this comment

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

I see. I would prefer an empty string instead, as it feels more natural, but still better than a crash.

Pointer validation should be done on the script side anyway (if you get it from somewhere), so we can state that providing a nullptr is UB (undefined behavior) and could change in the future. Today it may return (null), tomorrow an empty string, next week it could do something else. Would you be able to add a comment on top of this check, that this behavior is subject to change without notice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was always returning '(null)' and now I'm convinced there is no need to change it:

  • It will be 100% backward compatible if someone depended on it
  • Returning empty string will make more difficult to understand what is going on, (null) catches eye immediately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The pointer range check is meant to catch most frequent mistakes in form of:
print_formatted "My value %s" 0@ where %d should be used instead

@x87 x87 merged commit 1ff1b4b into master Oct 29, 2023
@MiranDMC MiranDMC deleted the error_messages_update branch October 29, 2023 15:14
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.

2 participants