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

CLOOP envelopes is wrong in IStatus [CORE6446] #6679

Closed
firebird-automations opened this issue Nov 16, 2020 · 13 comments
Closed

CLOOP envelopes is wrong in IStatus [CORE6446] #6679

firebird-automations opened this issue Nov 16, 2020 · 13 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: @aafemt

Attachments:
CORE-6446.artificial_test.cpp

If CLOOP envelope in IStatusImpl::getErrors()/getWarnings() catch an exception it returns null pointer and crash the engine.
It should return pointer to a static array the same as BaseStatusWrapper::catchException().

Commits: 52ea547 e039e01

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

assignee: Alexander Peshkov [ alexpeshkoff ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

Added attribute onError to interface's method in CLOOP.
[onError functionName]
is used when method should return non-default value in case of exception,
instead default one value returned by function functionName (w/o parameters) is used.

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

Fix Version: 4.0.0 [ 10931 ]

Fix Version: 3.0.8 [ 10960 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

What is an example of getErrors() / getWarnings() throwing?

@firebird-automations
Copy link
Collaborator Author

Commented by: @aafemt

class MyStatus: public Firebird::IStatusImpl<Status, MyStatusType>
{
const intptr_t* getErrors() const override
{
throw "Oops";
}
};

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

I attach artificial test, used for checking feature.
BTW, suppose that in a case when user has own Status implementation that's quite possible.

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

Attachment: CORE6446.artificial_test.cpp [ 13560 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

IMO this is very artificial and we are talking about really programming errors. (likewise, status could not protect about segfaults).

It's very easy to have potential throwable getErrors() / getWarnings() methods to catch exceptions themselves and return the failure status vector.

@firebird-automations
Copy link
Collaborator Author

Commented by: @aafemt

If user's getErrors/getWarnings implementations are supposed never throw then catch() in the envelopes is pointless and must be removed. If the catch() is there then throwing is expected and must be handled properly. That's what this ticket is about.

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

The envelop is generated in all methods.

It's certainly very easy for a developer to be a bit more careful when writing an implementation that is supposed to just deal with exceptions.

So far I had not see in Firebird code the problem, so it was certainly easy to make the thing work correctly.

Not a big things for me to continue discussing, it's just a thing that "fixes" no relevant problem.

How will the envelop deal with wrong getErrors() returning invalid pointers or pointers not adhering to status vector semantics?

Will be the next pass be adding IStatus argument to IStatus::getErrors/getWarnings?

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Resolved [ 5 ]

QA Status: No test => Cannot be tested

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Closed [ 6 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

If we request from "a developer to be a bit more careful when writing an implementation that is supposed to just deal with exceptions" what for are envelopes at all?

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

No branches or pull requests

2 participants