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

Crash when trying to access a non-existing member of a struct returned by a function #3105

Closed
ntsh-oni opened this issue Jan 8, 2023 · 1 comment · Fixed by #3108
Closed
Assignees

Comments

@ntsh-oni
Copy link
Contributor

ntsh-oni commented Jan 8, 2023

If you have a function returning a struct and you directly try to access a member from this struct from the function call, but this member does not actually exist, glslang, as a library, and glslangValidator will crash.

Shader example :

#version 460

struct A {
	float x;
};

A test() {
	return A(1.0);
}

void main() {
	test().z; // A.z does not exist, causes a crash
}

Crash happens during the parsing stage, in ParseHelper.cpp, in the handleDotDereference function :

auto baseSymbol = base;
while (baseSymbol->getAsSymbolNode() == nullptr)
baseSymbol = baseSymbol->getAsBinaryNode()->getLeft();
TString structName;
structName.append("\'").append(baseSymbol->getAsSymbolNode()->getName().c_str()).append( "\'");
error(loc, "no such field in structure", field.c_str(), structName.c_str());

The while loop is checking if baseSymbol is a TIntermSymbol, and if it is not, tries to dereference it as a binary node, which is nullptr in this case, causing a crash.

The difference between

test().z; // A.z does not exist, causes a crash

and

A c = A(1.0);
float b = c.z; // A.z does not exist, does not crash

is that baseSymbol is of type TIntermAggregate in the first case and TIntermSymbol in the second.

TIntermAggregate does not have a getAsSymbolNode nor a getAsBinaryNode member function and use the base class TIntermNode implementations instead, which both return nullptr.

getAsBinaryNode is implemented by TIntermBinary, so

auto baseSymbol = base;
while (baseSymbol->getAsSymbolNode() == nullptr) 
    baseSymbol = baseSymbol->getAsBinaryNode()->getLeft(); 

is trying to get a TIntermSymbol and estimates that if baseSymbol is not a TIntermSymbol, then it is a TIntermBinary, which is not the case here, as it is a TIntermAggregate.

One possible fix is to isolate both cases, one where baseSymbol is a TIntermBinary and one where baseSymbol is a TIntermAggregate, then getting the name of either the TIntermSymbol or the TIntermAggregate and write a different message when the error is from a TIntermAggregate :

auto baseSymbol = base;
while (baseSymbol->getAsSymbolNode() == nullptr) {
    if (baseSymbol->getAsBinaryNode() == nullptr) {
        break;
    }
    baseSymbol = baseSymbol->getAsBinaryNode()->getLeft();
}
TString structName;
if (baseSymbol->getAsSymbolNode() != nullptr) {
    structName.append("\'").append(baseSymbol->getAsSymbolNode()->getName().c_str()).append("\'");
    error(loc, "no such field in structure", field.c_str(), structName.c_str());
} else if (baseSymbol->getAsAggregate() != nullptr) {
    structName.append("\'").append(baseSymbol->getAsAggregate()->getName().c_str()).append("\'");
    structName.erase(structName.end() - 2);
    error(loc, "no such field in structure returned by function", field.c_str(), structName.c_str());
}

I don't know if any more types can reach this code path so I am not sure if it actually works in all cases.

With the example code :

ERROR: 0:15: 'z' : no such field in structure returned by function 'test'
ERROR: 0:15: '' : compilation terminated
ERROR: 2 compilation errors.  No code generated.
@greg-lunarg
Copy link
Contributor

I improvised off your code above. The non-symbol case didn't seem general enough to me, so I bailed and only printed the bad member name and not the struct expression for this case. It shouldn't crash now, and if someone wants to add more code to generate a nicer message for the non-symbol case, they are welcome.

kd-11 pushed a commit to kd-11/glslang that referenced this issue Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants