[GLES3] Protect against bogus glGetShaderInfoLog return values.

[lawnjelly]

On some buggy drivers GL_INFO_LOG_LENGTH returns incorrect values, which may lead to incorrectly filling in the log string. This could lead to uninitialized data being attempted to be printed and a crash. This PR zeros the array to ensure uninitialized data is not used.

Notes

• Reported by mori on rocket chat.
• This may or may not fix the crash, but seems good practice.

mori
6:30 AM
any openGL folks on? maybe lawnjelly ? i was debugging a web crash and it brought me to

godot/drivers/gles3/shader_gles3.cpp

Lines 317 to 328 in e38686f

	glGetShaderiv(spec.vert_id, GL_INFO_LOG_LENGTH, &iloglen);
	if (iloglen < 0) {
	glDeleteShader(spec.vert_id);
	glDeleteProgram(spec.id);
	spec.id = 0;
	ERR_PRINT("No OpenGL vertex shader compiler log.");
	} else {
	if (iloglen == 0) {
	iloglen = 4096; // buggy driver (Adreno 220+)
	}

. regarding GL_INFO_LOG_LENGTH, the spec says For implementations that support a shader compiler, params returns the number of characters in the information log for shader including the null termination character (i.e., the size of the character buffer required to store the information log). If shader has no information log, a value of 0 is returned. so i would think any code written against that spec would not have a <0 case, nor would it assume ==0 implies there are logs to be had
the issue is thickened by the fact emscripten, at least once, had a bug in this area emscripten-core/emscripten#11673

lawnjelly
This is probably for clayjohn but what was the crash call stack? Where was it crashing?
Maybe ilogmem should be memsetted to zero rather than just the last element.
If emscripten is returning 1 for length (bug), then not filling in ilogmem at all (because it should have returned zero), then it will try to print uninitialized memory.
So from the description memsetting may be the fix. But would have to see call stack of the crash.
If you create an issue that would be a good place to start.
I suspect that memsetting would be safer regardless.

[moribellamy]

Thanks! I'm still confused about the iloglen == 0 case. My reading of the spec is that this is allowed to happen when the shader compiler volunteers no log info. In that case, shouldn't we prefer execute ERR_PRINT("No OpenGL vertex shader compiler log."); ?

Also not sure that the +1 is needed. The standard suggests iloglen should come with a length suitable for a null-terminated string already.

I admit, this is not my area, and I understand if more defensive programming is required against buggy devices.

As written, this PR seems like a strict improvement to me.

[moribellamy]

PS: The code under modification seems to run fine in the web export, just with the unexpected behavior of sometimes printing Vertex Shader Compilation Failed: <empty_string>.

[lawnjelly]

@moribellamy That's a good point, I'll add a default string in the case of no information returned, as it makes more sense to the user and is easy to do. 👍

EDIT : Actually no, this should be for a separate PR, following the principle of one PR being as indivisible as possible. This PR is a bug fix and stands on its own.

Also not sure that the +1 is needed. The standard suggests iloglen should come with a length suitable for a null-terminated string already.

I didn't originally write this, but I think adding an +1 sounds sensible even if the spec says otherwise, for defensive reasons.

[akien-mga]

Thanks!