tobbebex's picture

GenRenderbuffers (and possibly others) writing outside provided out parameters

Project:The Open Toolkit library
Version:1.1-2014-01-02
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:closed
Description

glGenRenderbuffers second parameter is supposed to be a pointer to an array where the generated buffer names are stored.
In OpenTK, there are two overloads that take "out int" and "out uint" respectively for this parameter. Consider this example:

class A
{
    private int myRenderBuffer;
    private int myImportantOtherField;
 
    public A() 
    {
        myImportantOtherField = 378;
        GL.GenRenderbuffers(2, out myRenderBuffer);
        // At this point, myRenderBuffer is likely set to 1, and myImportantOtherField has been overwritten with the value 2
    }
}

This example is actually rather nice, if you dont have a second int field directly after the other one, segmentation faults and other nasty things are likely to happen instead.

If those overloads are there for conveniance when you only create a single buffer, the parameter n shouldn't be exposed.


Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
the Fiddler's picture

#1

Version:1.0.0-rc1» 1.x-dev
Status:open» fixed

Thanks for reporting this, I have now added single-object overloads to all Gen* and Delete* functions:

int id = GL.GenRenderbuffer(); // Creates a single renderbuffer object name
GL.DeleteRenderbuffer(id);

So I consider this issue fixed as of r3123.

The 'out' parameter is designed this way so you can do things like:

var bar = new Vector4();
GL.GetBar(4, out bar.X); // Fills X, Y, Z, W

Without the 'out' overload, you would have to allocate an array or use unsafe code instead.

In this case, it is the programmer's responsibility to ensure correct semantics. If you try to write >n items to a n-sized array, then all bets are off (it's a classic out-of-bounds access bug).

tobbebex's picture

#2

I wouldn't call it "classic" in c#. There you are used to get an exception when you try to access an array out of bounds, not silently writing over the consecutive bytes.

The method you describe of filling an struct seem unsafe to me. Usually you would require the caller to pin the struct down so the garbage collector isn't moving it while the native call is being made, and then you would have got an IntPtr you could use instead. I think that is more idiomatic in .net as well.

Regarding the overload taking an int or uint array by the way: since you have to pre allocate it, there shouldn't be a reason you need to send in the length as the parameter n, it could just use the length of the array. Guess you don't want to change the interface at this point, but maybe room for an additional overload?

Cheers
Tobias

the Fiddler's picture

#3

It's a "classic" bug in native APIs, like OpenGL.

OpenTK pins arrays, out and ref parameters automatically for the duration of the call. (If you need more granular pinning, you can use pointer / IntPtr overloads).

In an ideal world, the n and length parameters would be determined automatically from the length of the array or ArraySegment. Unfortunately, the OpenGL spec was not designed with this in mind, so someone would need to locate all length parameters and add them to the gloverrides.xml file. Personally, I consider the amount of work involved prohibitive (just try searching for "COMPSIZE" in gl.spec) but I would love to be proven wrong!

tobbebex's picture

#4

I guess you're right that a close map to the original spec is a good thing after all. I just have to get used to treating out and ref parameters as pointers. :)

Thanks for all your great work on OpenTK!

the Fiddler's picture

#5

Version:1.x-dev» 1.1-2014-01-02
Status:fixed» closed

Closing bugs fixed in OpenTK 1.1.

If this is still an issue please file a new bug report at https://github.com/opentk/opentk/issues