Qiankun: is there some test case that hits this?

(In reply to Tapani Pälli from comment #1)
> Qiankun: is there some test case that hits this?

I met this issue when I running a WebGL conformance test: https://www.khronos.org/registry/webgl/sdk/tests/conformance/programs/program-infolog.html?webglVersion=1. Now, I workarounded the bug in chrome, but the bug in mesa is still existed. 

To reproduce the bug:
(1) you can download a stable chrome. Note: canary chrome cannot reproduce the bug.
(2) run chrome --enable-unsafe-es3-apis --ignore-gpu-blacklist https://www.khronos.org/registry/webgl/sdk/tests/conformance/programs/program-infolog.html?webglVersion=1.
(3) All tests should pass.

I think this test is wrong, and there is a GLES test that expects the behavior that we do. glGetProgramIngoLog always writes at least a NUL character, so the length returned by this quest must be at least 1.

(In reply to Ian Romanick from comment #3)
> I think this test is wrong, and there is a GLES test that expects the
> behavior that we do. glGetProgramIngoLog always writes at least a NUL
> character, so the length returned by this quest must be at least 1.

Which GLES test is expecting current behaviour? I did not see any regressions when testing with this suggested change:

https://lists.freedesktop.org/archives/mesa-dev/2016-August/126144.html

(In reply to Ian Romanick from comment #3)
> I think this test is wrong, and there is a GLES test that expects the
> behavior that we do. glGetProgramIngoLog always writes at least a NUL
> character, so the length returned by this quest must be at least 1.

Agree with you glGetProgramInfoLog always writes at least a NULL character. But GetProgramiv with pname is INFO_LOG_LENGTH should return 0 for this case according the spec: "If there is no info log, zero is returned.". If length returned must be at least 1, when 0 will be returned as spec says?

The spec also says, "If pname is INFO_LOG_LENGTH , the length of the info log, including a null terminator, is returned."  If this query returns 0, then glGetProgramInfoLog CANNOT write anything to the buffer... and it currently does.

(In reply to Ian Romanick from comment #6)
> The spec also says, "If pname is INFO_LOG_LENGTH , the length of the info
> log, including a null terminator, is returned."  If this query returns 0,
> then glGetProgramInfoLog CANNOT write anything to the buffer... and it
> currently does.

I think returned value of GetProgramiv() can be 0. It doesn't affect the real length of the info log buffer. Driver can still return a null terminator string.

(In reply to Qiankun Miao from comment #7)
> (In reply to Ian Romanick from comment #6)
> > The spec also says, "If pname is INFO_LOG_LENGTH , the length of the info
> > log, including a null terminator, is returned."  If this query returns 0,
> > then glGetProgramInfoLog CANNOT write anything to the buffer... and it
> > currently does.
> 
> I think returned value of GetProgramiv() can be 0. It doesn't affect the
> real length of the info log buffer. Driver can still return a null
> terminator string.

That is absolutely false.  The whole point of the glGetProgramiv(GL_INFO_LOG_LENGTH) query is so that the application can allocate a buffer to store the data that will be returned by glGetProgramInfoLog.  Having those mismatch, under any circumstances, is wrong to the point that it could create a buffer overrun security problem.

I guess the point is that you can't change one and not change the other.  I will not budge on that.

I also think it is wrong for glGetProgramInfoLog to not write a NUL when the info log is empty.  An application that does something like


    glGetProgramInfoLog(prog, max_length, NULL, log);
    printf("Info log:\n%s\n", log);

will print whatever garbage happens to be in log.  You may argue that the application is incorrect, but this usage is very, very common (among users of glGetProgramInfoLog).

Also... has anyone checked what other implementations do using, say, a piglit test?  Something other than WebGL tests.

(In reply to Ian Romanick from comment #10)
> Also... has anyone checked what other implementations do using, say, a
> piglit test?  Something other than WebGL tests.

FYI:
NVidia 349.16 behaves the same as Mesa. NVidia 364.19 returns 0.

(In reply to Ian Romanick from comment #9)
> I guess the point is that you can't change one and not change the other.  I
> will not budge on that.
> 
> I also think it is wrong for glGetProgramInfoLog to not write a NUL when the
> info log is empty.  An application that does something like
> 
> 
>     glGetProgramInfoLog(prog, max_length, NULL, log);
>     printf("Info log:\n%s\n", log);
> 
> will print whatever garbage happens to be in log.  You may argue that the
> application is incorrect, but this usage is very, very common (among users
> of glGetProgramInfoLog).

max_length should make sure "log" doesn't overflow, otherwise it's a bug of glGetProgramInfoLog implementation. 

Here is the usage of these two apis in chrome:
void Program::UpdateLogInfo() {
  GLint max_len = 0;
  glGetProgramiv(service_id_, GL_INFO_LOG_LENGTH, &max_len);
  if (max_len == 0) {
    set_log_info(nullptr);
    return;
  }
  std::unique_ptr<char[]> temp(new char[max_len]);
  GLint len = 0;
  glGetProgramInfoLog(service_id_, max_len, &len, temp.get());
  DCHECK(max_len == 0 || len < max_len);
  DCHECK(len == 0 || temp[len] == '\0');
  std::string log(temp.get(), len);
  log = ProcessLogInfo(log);
  set_log_info(log.empty() ? nullptr : log.c_str());
}

Application should handle max_len returned 0. If application didn't handle that, glGetProgramInfoLog should make sure no buffer overflow for "max_len == 0".

(In reply to Qiankun Miao from comment #11)
> (In reply to Ian Romanick from comment #10)
> > Also... has anyone checked what other implementations do using, say, a
> > piglit test?  Something other than WebGL tests.
> 
> FYI:
> NVidia 349.16 behaves the same as Mesa. NVidia 364.19 returns 0.

When the GL_INFO_LOG_LENGTH query returns 0, what does the following code output?

    char buffer[256];

    strcpy(buffer, "Nothing was written.");
    glGetProgramInfoLog(prog, sizeof(buffer), NULL, buffer);

    printf("%s\n", buffer);

(In reply to Ian Romanick from comment #13)
> (In reply to Qiankun Miao from comment #11)
> > (In reply to Ian Romanick from comment #10)
> > > Also... has anyone checked what other implementations do using, say, a
> > > piglit test?  Something other than WebGL tests.
> > 
> > FYI:
> > NVidia 349.16 behaves the same as Mesa. NVidia 364.19 returns 0.
> 
> When the GL_INFO_LOG_LENGTH query returns 0, what does the following code
> output?
> 
>     char buffer[256];
> 
>     strcpy(buffer, "Nothing was written.");
>     glGetProgramInfoLog(prog, sizeof(buffer), NULL, buffer);
> 
>     printf("%s\n", buffer);

buffer would be:"\000othing was written.", '\000' <repeats xx times>
So printf will print an empty line.

Ian, what would be the final verdict for this one?

NVIDIA's behavior is wrong and can lead to security problems in applications.  I will not allow my driver to do that.  If the spec demands this behavior, it is wrong too.

There has been a couple weeks of discussion about this inside Khronos (see Khronos internal bug #16110).

A lot of the discussion focused on which option would be least likely to be used incorrectly by developers.  Since glGetProgramInfoLog takes a maxLength parameter, overruns should be unlikely.  I'm still not 100% comfortable with it, but I can live with it.

The conclusion is as follows:

1. In the initial state, the info log is empty.  As of this writing, the spec says there is "no info log."

2. If the info log is empty, the GL_INFO_LOG_LENGTH query should return 0.

3. If maxLength passed to glGetProgramInfoLog is > 0, the buffer pointed to by infoLog will be NUL terminated even if GL_INFO_LOG_LENGTH returned 0.  The spec be clarified to make this last point explicit.

Guess who gets to write CTS tests to exercise all of this behavior?  Yeah, I do.

We should implement the behavior described by the pending spec changes.  I believe this matches the expectations of the WebGL test referenced in comment #2.

Tapani: Are you still available to do this?

commit ec4e71f75e9b8a1c427994efa32a61593e3172f9
Author: Tapani Pälli <tapani.palli@intel.com>
Date:   Wed Aug 17 10:37:45 2016 +0300

    mesa: fix empty program log length
    
    In case we have empty log (""), we should return 0. This fixes
    Khronos WebGL conformance test 'program-infolog'.
    
    From OpenGL ES 3.1 (and OpenGL 4.5 Core) spec:
       "If pname is INFO_LOG_LENGTH , the length of the info log, including
        a null terminator, is returned. If there is no info log, zero is
        returned."
    
    v2: apply same fix for get_shaderiv and _mesa_GetProgramPipelineiv (Ian)
    
    Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
    Reviewed-by: Iago Toral Quiroga <itoral@igalia.com> (v1)
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
    Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97321
    Cc: "13.0" <mesa-stable@lists.freedesktop.org>