[PATCH wayland 2/3] Add version information to wl_message signatures.

[Jason Ekstrand]

On Thu, Jun 27, 2013 at 8:31 PM, Bill Spitzak <spitzak at gmail.com> wrote:

> Jason Ekstrand wrote:
>
>  That is exactly what this patch allows you to do.  More particularly,
>> this patch protects the compositor from bad clients that attempt to use
>> requests that are not supported by the compositor.  In the current
>> implementation, the compositor simply crashes.
>>
>
> I'm confused as to why crashes cannot be avoided in much simpler ways.
>
> For instance if version 1 of FooObject acted like the "twiddle" setting
> was 5. Then a new version is made because it looked better to default
> "twiddle" to zero and add a "set_twiddle" message. I would expect the
> implementation in a compositor that understands the new version to be
> something like this:
>
>     struct FooObject {
>        int twiddle;
>        int version;
>     };
>
>     FooObject* makeFooObject(int version) {
>        x = new FooObject;
>        x.twiddle = version < 2 ? 5 : 0;
>        x.version = version;
>     }
>
>     // this is the function put in dispatch table:
>     set_twiddle(FooObject* x, int value) {
>        if (version < 2) error; // optional if the change is harmless
>        x.twiddle = value;
>     }
>
> You seem to be writing it as though the compositor does this:
>
>     struct FooObject1 {};
>
>     struct FooObject2 { int twiddle; }
>
>     FooObject* makeFooObject(int version) {
>         if (version < 2) {
>              x = new FooObject1;
>              return x;
>         } else {
>              x = new FooObject2;
>              x.twiddle = 0;
>              return x;
>         }
>     }
>
>     set_twiddle(void* x, int value) {
>         ((FooObject2*)x)->twiddle = value; // crashes if it is a
> FooObject1!
>     }
>
> I find that unlikely. However even if there are different structures, if
> they share a "version" at the start the implementation can still check the
> version number like in my first example.
>
> This also does not solve methods that have to act different depending on
> the version. Which, if they are different structures, is true for all the
> methods that are in common. So adding "fail" as something it does depending
> on version seems easy.
>

I don't think you understand the problem that I am trying to solve.  Allow
me to try and explain it better.  I'll use your example above.  Let's say
that wl_foo has a set_bar request and a set_twiddle request that was added
in version 2.  Now let's say we have an older compositor that was written
before anyone ever decided that the client should be able to set the
twiddle.  Further suppose that we have a broken client that asks for
version 2, doesn't properly negotiate, and sets the twiddle regardless.  As
it currently stands, the compositor interface implementation would look
something like this

wl_foo_interface foo_implementation = {
    set_bar
};

while a more modern compositor implementation would look like this:

wl_foo_interface foo_implementation = {
    set_bar,
    set_twiddle
};

As it stands, libwayland does not checking of requests beyond a simple
check to see request n is devined in the wl_interface object that was built
into the library.  Let's see what happens when the client calls set_twiddle
(opcode 1) in the current library (without this patch).  In this case the
following happens:

1) libwayland looks up the object and gets the interface
2) libwayland checks to see if a wl_foo has at least 2 requests.  The
answer is "yes"
3) libwayland blindly looks up the 2nd entry in foo_implementation and
attempts to call it.

At this point we will segfault because the 2nd entry will either be null or
some other garbage value depending on the version of libwayland that the
compositor was compiled against.  This patch adds a step between 2 and 3:

2.5) libwayland checks to see of the version of this particular wl_foo
resource supports set_twiddle.  The answer is "no"

In this way it won't get to step 3 because the compositor doesn't provide
set_twiddle.  Instead, it will throw a protocol error and disconnect the
client.  I hope this makes more sense.


>      To match your use of atoi it would be better to skip all the leading
>>     digits (possibly in other code where you know you are at the start
>>     of the signature), then use this old version of the code.
>>
>>
>> I thought about doing that but this is less invasive. Also, I like this
>> method of counting better anyway because it's a bit more robust.  Suppose
>> we add in some other type-specifier than "?".  In that case, the old
>> implementation wouldn't work anyway.
>>
>
> I am much more worried about a new argument type being added. This will
> miscount them unless it is rewritten.
>

I thought about this and I actually consider it an advantage.  It forces
future programmers to think about how it interacts with the rest of the
signature and prevents accidental breakage due to inconsistent signature
parsing.

--Jason Ekstrand
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20130628/4a8e1358/attachment.html>