Bug 97321 - Query INFO_LOG_LENGTH for empty info log should return 0
Summary: Query INFO_LOG_LENGTH for empty info log should return 0
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: medium normal
Assignee: Tapani Pälli
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-12 08:24 UTC by Qiankun Miao
Modified: 2016-11-25 05:34 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Qiankun Miao 2016-08-12 08:24:32 UTC
When calling GetProgramiv with 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. See the above description in OpenGL spec 4.5, glspec45.core.pdf page 157.

But 1 is returned in Mesa for empty info log which doesn't align with spec.
Comment 1 Tapani Pälli 2016-08-17 06:38:58 UTC
Qiankun: is there some test case that hits this?
Comment 2 Qiankun Miao 2016-08-17 07:02:01 UTC
(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.
Comment 3 Ian Romanick 2016-08-17 08:18:28 UTC
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.
Comment 4 Tapani Pälli 2016-08-17 08:20:56 UTC
(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
Comment 5 Qiankun Miao 2016-08-17 08:29:51 UTC
(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?
Comment 6 Ian Romanick 2016-08-17 16:11:31 UTC
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.
Comment 7 Qiankun Miao 2016-08-18 02:12:50 UTC
(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.
Comment 8 Ian Romanick 2016-08-18 08:29:49 UTC
(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.
Comment 9 Ian Romanick 2016-08-18 08:33:54 UTC
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).
Comment 10 Ian Romanick 2016-08-18 08:36:21 UTC
Also... has anyone checked what other implementations do using, say, a piglit test?  Something other than WebGL tests.
Comment 11 Qiankun Miao 2016-08-18 08:41:44 UTC
(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.
Comment 12 Qiankun Miao 2016-08-18 09:34:01 UTC
(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".
Comment 13 Ian Romanick 2016-08-18 16:04:05 UTC
(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);
Comment 14 Qiankun Miao 2016-08-19 09:19:02 UTC
(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.
Comment 15 Tapani Pälli 2016-09-01 08:41:34 UTC
Ian, what would be the final verdict for this one?
Comment 16 Ian Romanick 2016-11-04 23:07:23 UTC
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.
Comment 17 Ian Romanick 2016-11-16 20:27:20 UTC
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?
Comment 18 Tapani Pälli 2016-11-25 05:34:04 UTC
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>


bug/show.html.tmpl processed on Jan 16, 2017 at 17:21:22.
(provided by the Example extension).