We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 2615 - patch for small invalid read in opengles
Summary: patch for small invalid read in opengles
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: HG 2.1
Hardware: x86_64 Linux
: P2 normal
Assignee: Ryan C. Gordon
QA Contact: Sam Lantinga
URL:
Keywords: target-2.0.4, triage-2.0.4
Depends on:
Blocks:
 
Reported: 2014-06-29 07:28 UTC by Sylvain
Modified: 2015-05-26 14:45 UTC (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sylvain 2014-06-29 07:28:15 UTC 
I have this one (on linux with GLES 1) :

==4218== Conditional jump or move depends on uninitialised value(s)
==4218==    at 0x4F58EAE: SDL_GL_SetAttribute_REAL (SDL_video.c:2715)
==4218==    by 0x4EBF113: GLES_CreateRenderer (SDL_render_gles.c:415)
==4218==    by 0x4EADA7D: SDL_CreateRenderer_REAL (SDL_render.c:272)

which is :

GLES_CreateRenderer
...
 412 error:
 413     if (changed_window) {
 414         /* Uh oh, better try to put it back... */
 415         SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK, profile_mask);
 416         SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, major);
 417         SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, minor);



But the "SDL_GL_GetAttribute" has previously failed because no "glGetIntegerv".
so "int profile_mask, major, minor;" are not initialized.

int
SDL_GL_GetAttribute(SDL_GLattr attr, int *value)
{
#if SDL_VIDEO_OPENGL || SDL_VIDEO_OPENGL_ES || SDL_VIDEO_OPENGL_ES2
    void (APIENTRY * glGetIntegervFunc) (GLenum pname, GLint * params);
    GLenum(APIENTRY * glGetErrorFunc) (void);
    GLenum attrib = 0;
    GLenum error = 0;

    glGetIntegervFunc = SDL_GL_GetProcAddress("glGetIntegerv");
    if (!glGetIntegervFunc) {
        printf("No glGetIntegerv!\n"); // This occured !
        return -1;
    }


Then, the creation of the renderer also failed, so putting back the values is performed with uninitialized memory.


Solution:
========

easy: 
====
initialize :
int profile_mask, major, minor;

better:
======
Add a flag : 
SDL_bool attributes_retrieved = SDL_FALSE;

check for "SDL_GL_GetAttribute" being not -1

and in err:
if (changed_window && attributes_retrieved)


And also better :
=================

Inside "SDL_GL_GetAttribute", move the SDL_GL_GetProcAddress("glGetIntegerv"); 
where is it actually required !
(this is not required to have profile_mask, major, minor)
Comment 1 Ryan C. Gordon 2015-02-19 05:22:23 UTC 
Marking a large number of bugs with the "triage-2.0.4" keyword at once. Sorry if you got a lot of email from this. This is to help me sort through some bugs in regards to a 2.0.4 release. We may or may not fix this bug for 2.0.4, though!
Comment 2 Ryan C. Gordon 2015-04-07 04:57:56 UTC 
(sorry if you get a lot of copies of this email, I'm marking several bugs at once)

Marking bugs for the (mostly) final 2.0.4 TODO list. This means we're hoping to resolve this bug before 2.0.4 ships if possible. In a perfect world, the open bug count with the target-2.0.4 keyword is zero when we ship.

(Note that closing a bug report as WONTFIX, INVALID or WORKSFORME might still happen.)

--ryan.
Comment 3 Ryan C. Gordon 2015-05-26 14:45:30 UTC 
This is fixed in https://hg.libsdl.org/SDL/rev/26a7259520cd (I moved the test for glGetIntegerv() down to where it is actually used).

We'll likely need to fix this properly for actually getting those attributes at some point, but this is good enough for now.

--ryan.