<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 27, 2013 at 8:31 PM, Bill Spitzak <span dir="ltr"><<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">Jason Ekstrand wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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.<br>
</blockquote>
<br></div>
I'm confused as to why crashes cannot be avoided in much simpler ways.<br>
<br>
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:<br>
<br>
struct FooObject {<br>
int twiddle;<br>
int version;<br>
};<br>
<br>
FooObject* makeFooObject(int version) {<br>
x = new FooObject;<br>
x.twiddle = version < 2 ? 5 : 0;<br>
x.version = version;<br>
}<br>
<br>
// this is the function put in dispatch table:<br>
set_twiddle(FooObject* x, int value) {<br>
if (version < 2) error; // optional if the change is harmless<br>
x.twiddle = value;<br>
}<br>
<br>
You seem to be writing it as though the compositor does this:<br>
<br>
struct FooObject1 {};<br>
<br>
struct FooObject2 { int twiddle; }<br>
<br>
FooObject* makeFooObject(int version) {<br>
if (version < 2) {<br>
x = new FooObject1;<br>
return x;<br>
} else {<br>
x = new FooObject2;<br>
x.twiddle = 0;<br>
return x;<br>
}<br>
}<br>
<br>
set_twiddle(void* x, int value) {<br>
((FooObject2*)x)->twiddle = value; // crashes if it is a FooObject1!<br>
}<br>
<br>
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.<br>
<br>
This also does not solve methods that have to act different depending on<br>
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.<br><div class="im"></div></blockquote>
<div><br></div><div>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<br>
<br></div><div>wl_foo_interface foo_implementation = {<br></div><div> set_bar<br>};<br><br></div><div>while a more modern compositor implementation would look like this:<br><br><div>wl_foo_interface foo_implementation = {<br>
</div> set_bar,<br></div><div> set_twiddle<br></div><div>};<br><br></div><div>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:<br>
<br></div><div>1) libwayland looks up the object and gets the interface<br></div><div>2) libwayland checks to see if a wl_foo has at least 2 requests. The answer is "yes"<br></div><div>3) libwayland blindly looks up the 2nd entry in foo_implementation and attempts to call it.<br>
<br></div><div>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:<br>
<br></div><div>2.5) libwayland checks to see of the version of this particular wl_foo resource supports set_twiddle. The answer is "no"<br><br></div><div>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.<br>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
To match your use of atoi it would be better to skip all the leading<br>
digits (possibly in other code where you know you are at the start<br>
of the signature), then use this old version of the code.<br>
<br>
<br>
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.<br>
</blockquote>
<br></div>
I am much more worried about a new argument type being added. This will miscount them unless it is rewritten.<br></blockquote><div><br></div><div>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.<br>
<br></div><div>--Jason Ekstrand <br></div></div><br><br></div></div>