Add the web safe shader analyzer under the SH_WEB_SAFE compile flag.

Add the web safe shader analyzer under the SH_WEB_SAFE compile flag.

Related files are under the new directory src/compiler/websafe.

Description of the algorithm:
http://code.google.com/p/mvujovic/wiki/ShaderControlFlowAnalysis

Background:
The web safe shader analyzer is one potential solution to timing attacks on textures containing cross-domain content or user agent data. This kind of analysis could be useful for both WebGL and CSS Shaders.

The web safe shader analyzer will reject a shader if it uses texture dependent data to affect control flow. 

Other ways of affecting shader timing such as using NaNs in basic arithmetic operations or using built-in functions (e.g. atan) with different inputs are still under investigation. Checks for these cases are not yet implemented, but this code could be easily extended to cover those cases once we have more information.

BUG=http://code.google.com/p/angleproject/issues/detail?id=329
TEST=Run the translator with "-w" on a shader to use the SH_WEB_SAFE flag.
Additionally, use the "-d" flag to print out the dependency graph used to validate web safe fragment shaders.

[kbr1, 2012-05-08 22:33:07]

Added nicolas and alokp to CC list.

[Alok Priyadarshi, 2012-05-09 21:01:21]

I have not looked at this in detail. I have a higher level question and comment:

1. Should SH_VALIDATE_LOOP_INDEXING be folded into SH_WEB_SAFE?
2. Nit: Formatting looks messed up in a couple places. Some of the line lengths
exceed 100 chars (ANGLE style guide)

[mvujovic, 2012-05-09 21:48:16]

On 2012/05/09 21:01:21, Alok Priyadarshi wrote:
> I have not looked at this in detail. I have a higher level question and
comment:

Thanks for looking at this!

> 1. Should SH_VALIDATE_LOOP_INDEXING be folded into SH_WEB_SAFE?

I see what you're saying. The SH_WEB_SAFE flag was intended to be used when
compiling a WebGL shader. That is, the shader spec flag should be set to
SH_WEBGL_SPEC, meaning SH_VALIDATE_LOOP_INDEXING should be turned on.

Maybe we can either:
(1) If SH_WEB_SAFE is set, force the shader spec to be SH_WEBGL_SPEC. 
(2) Throw an error when SH_WEB_SAFE is used and the spec is not SH_WEBGL_SPEC.

What do you think?

> 2. Nit: Formatting looks messed up in a couple places. Some of the line
lengths
> exceed 100 chars (ANGLE style guide)

Ok- I'll take a look and fix that up.

[kbr1, 2012-05-10 00:12:20]

On 2012/05/09 21:48:16, mvujovic wrote:
> On 2012/05/09 21:01:21, Alok Priyadarshi wrote:
> > I have not looked at this in detail. I have a higher level question and
> comment:
> 
> Thanks for looking at this!
> 
> > 1. Should SH_VALIDATE_LOOP_INDEXING be folded into SH_WEB_SAFE?
> 
> I see what you're saying. The SH_WEB_SAFE flag was intended to be used when
> compiling a WebGL shader. That is, the shader spec flag should be set to
> SH_WEBGL_SPEC, meaning SH_VALIDATE_LOOP_INDEXING should be turned on.
> 
> Maybe we can either:
> (1) If SH_WEB_SAFE is set, force the shader spec to be SH_WEBGL_SPEC. 
> (2) Throw an error when SH_WEB_SAFE is used and the spec is not SH_WEBGL_SPEC.
> 
> What do you think?

(2) sounds like less surprising behavior. The behavior should be documented
either way.

http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h#...
include/GLSLANG/ShaderLang.h:108: SH_WEB_SAFE                   = 0x0200,
Add a newline and documentation before these new flags.

[mvujovic, 2012-05-10 16:58:57]

On 2012/05/10 00:12:20, kbr1 wrote:
> On 2012/05/09 21:48:16, mvujovic wrote:
> > On 2012/05/09 21:01:21, Alok Priyadarshi wrote:
> > > I have not looked at this in detail. I have a higher level question and
> > comment:
> > 
> > Thanks for looking at this!
> > 
> > > 1. Should SH_VALIDATE_LOOP_INDEXING be folded into SH_WEB_SAFE?
> > 
> > I see what you're saying. The SH_WEB_SAFE flag was intended to be used when
> > compiling a WebGL shader. That is, the shader spec flag should be set to
> > SH_WEBGL_SPEC, meaning SH_VALIDATE_LOOP_INDEXING should be turned on.
> > 
> > Maybe we can either:
> > (1) If SH_WEB_SAFE is set, force the shader spec to be SH_WEBGL_SPEC. 
> > (2) Throw an error when SH_WEB_SAFE is used and the spec is not
SH_WEBGL_SPEC.
> > 
> > What do you think?
> 
> (2) sounds like less surprising behavior. The behavior should be documented
> either way.

Agreed. I just uploaded (2), and the behavior is documented above the
SH_WEB_SAFE flag.

> http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h
> File include/GLSLANG/ShaderLang.h (right):
> 
>
http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h#...
> include/GLSLANG/ShaderLang.h:108: SH_WEB_SAFE                   = 0x0200,
> Add a newline and documentation before these new flags.

Done.

[gmanchromium, 2012-05-10 19:44:20]

http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h#...
include/GLSLANG/ShaderLang.h:108: SH_WEB_SAFE                   = 0x0200,
Just my bikeshedding but is WEB_SAFE the right name? That seems to invite people
saying other shaders are not SAFE which I think we'd prefer not to insinuate

[mvujovic, 2012-05-10 21:07:58]

On 2012/05/10 19:44:20, gmanchromium wrote:
> http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h
> File include/GLSLANG/ShaderLang.h (right):
> 
>
http://codereview.appspot.com/6195062/diff/9001/include/GLSLANG/ShaderLang.h#...
> include/GLSLANG/ShaderLang.h:108: SH_WEB_SAFE                   = 0x0200,
> Just my bikeshedding but is WEB_SAFE the right name? That seems to invite
people
> saying other shaders are not SAFE which I think we'd prefer not to insinuate

That's a good point. How about calling them "strict shaders" instead of "web
safe shaders"?
Then, we could change SH_WEB_SAFE to SH_STRICT.

Here are some other names I was thinking about:
SH_VALIDATE_TIMING
SH_TEXTURE_INDEPENDENT_TIME
SH_TIMING_ANALYSIS
SH_TIMING_RESTRICTIONS

[nicolas, 2012-05-18 14:18:15]

This is a really interesting concept. I went through the patches and I have no
real concerns. It's well isolated from the rest of the compiler. Nice job!

If this becomes a standard, we could consider integrating the dependency graph
into the AST itself. That way the analysis information could also be used to
optimize the code before sending it to the translator or the graphics driver's
compiler.

[mvujovic, 2012-05-20 19:41:39]

On 2012/05/18 14:18:15, nicolas wrote:
> This is a really interesting concept. I went through the patches and I have no
> real concerns. It's well isolated from the rest of the compiler. Nice job!

Thanks, Nicolas!

> If this becomes a standard, we could consider integrating the dependency graph
> into the AST itself. That way the analysis information could also be used to
> optimize the code before sending it to the translator or the graphics driver's
> compiler.

For sure. The current dependency graph would be good at identifying dead code
and probably some other scenarios. It would be interesting to compile some
shaders on the graphics driver and see where we can do a better job using the
dependency graph information. I thinking we might need to add a concept of time
in the dependency graph (i.e. when symbols start depending on each other) to
enable more significant optimizations. Sounds like interesting research :)

[mvujovic, 2012-05-22 15:15:09]

> On 2012/05/10 19:44:20, gmanchromium wrote:
> Just my bikeshedding but is WEB_SAFE the right name? That seems to invite
> people saying other shaders are not SAFE which I think we'd prefer not to
insinuate

Any opinions on calling these "strict shaders" instead of "web safe shaders"? I
can update the patch to use SH_STRICT instead of SH_WEB_SAFE if you guys are
okay with that.

Thanks,
Max

[bjacob_mozilla.com, 2012-05-22 15:29:23]

----- Original Message -----
> > On 2012/05/10 19:44:20, gmanchromium wrote:
> > Just my bikeshedding but is WEB_SAFE the right name? That seems to
> invite
> > people saying other shaders are not SAFE which I think we'd prefer
> > not
> to insinuate
> 
> Any opinions on calling these "strict shaders" instead of "web safe
> shaders"? I can update the patch to use SH_STRICT instead of
> SH_WEB_SAFE
> if you guys are okay with that.

I would go for a the most specific name possible. For example:
 SH_NON_TIMEABLE
 SH_TIMING_ATTACK_PROOF
 SH_CONSTANT_TIME
 ...etc?

Benoit

> 
> Thanks,
> Max
> 
> http://codereview.appspot.com/6195062/
> 

[mvujovic, 2012-05-22 16:25:30]

On 2012/05/22 15:29:23, bjacob_mozilla.com wrote:
>
> ----- Original Message -----
> > > On 2012/05/10 19:44:20, gmanchromium wrote:
> > > Just my bikeshedding but is WEB_SAFE the right name? That seems to invite
> > > people saying other shaders are not SAFE which I think we'd prefer
> > > not to insinuate
> >
> > Any opinions on calling these "strict shaders" instead of "web safe
> > shaders"? I can update the patch to use SH_STRICT instead of
> > SH_WEB_SAFE
> > if you guys are okay with that.
>
> I would go for a the most specific name possible. For example:
>  SH_NON_TIMEABLE
>  SH_TIMING_ATTACK_PROOF
>  SH_CONSTANT_TIME
>  ...etc?
>
> Benoit

Thanks for chiming in, Benoit :)

Out of those, I think SH_TIMING_ATTACK_PROOF most specifically describes what
this flag is trying to do. 
I'm worried it might imply too much security for the experimental nature of this
flag, though.

I'm liking SH_TIMING_RESTRICTIONS the best right now because it communicates
that:
1) The flag enforces restrictions.
2) The restrictions relate to timing.
3) The restrictions don't necessarily make us safe or unsafe.
4) The restrictions are intentionally vague. Current restrictions relate to
control flow, but future restrictions could disallow texture derived values in
some built-in functions.

Thoughts?

[bjacob_mozilla.com, 2012-05-22 16:57:05]

----- Original Message -----
> On 2012/05/22 15:29:23, bjacob_mozilla.com wrote:
> 
> > ----- Original Message -----
> > > > On 2012/05/10 19:44:20, gmanchromium wrote:
> > > > Just my bikeshedding but is WEB_SAFE the right name? That seems
> > > > to
> invite
> > > > people saying other shaders are not SAFE which I think we'd
> > > > prefer
> > > > not to insinuate
> > >
> > > Any opinions on calling these "strict shaders" instead of "web
> > > safe
> > > shaders"? I can update the patch to use SH_STRICT instead of
> > > SH_WEB_SAFE
> > > if you guys are okay with that.
> 
> > I would go for a the most specific name possible. For example:
> >   SH_NON_TIMEABLE
> >   SH_TIMING_ATTACK_PROOF
> >   SH_CONSTANT_TIME
> >   ...etc?
> 
> > Benoit
> 
> Thanks for chiming in, Benoit :)
> 
> Out of those, I think SH_TIMING_ATTACK_PROOF most specifically
> describes
> what this flag is trying to do.
> I'm worried it might imply too much security for the experimental
> nature
> of this flag, though.
> 
> I'm liking SH_TIMING_RESTRICTIONS the best right now because it
> communicates that:

SH_TIMING_RESTRICTIONS works for me.

Benoit

> 1) The flag enforces restrictions.
> 2) The restrictions relate to timing.
> 3) The restrictions don't necessarily make us safe or unsafe.
> 4) The restrictions are intentionally vague. Current restrictions
> relate
> to control flow, but future restrictions could disallow texture
> derived
> values in some built-in functions.
> 
> Thoughts?
> 
> http://codereview.appspot.com/6195062/
> 

[dgkoch, 2012-05-22 17:57:49]

I definitely like the name SH_TIMING_RESTRICTIONS better than SH_WEBGL_SAFE. 
Seems like you should also rename a number of the classes and function as well
to reflect that name.

Another suggestion about new files: Right now they are all in a "websafe"
folder.  I'd suggest putting all the dependency graph related ones into a
separate folder "depgen" or something, and then another one for the webgl
specific validations.

http://codereview.appspot.com/6195062/diff/14001/AUTHORS
File AUTHORS (right):

http://codereview.appspot.com/6195062/diff/14001/AUTHORS#newcode16
AUTHORS:16: Adobe Systems Inc.
I'm not sure Adobe should be added to the AUTHORS file, as we haven't done this
for other organizations (but it definitely should to the CONTRIBUTORS).

Ken?

http://codereview.appspot.com/6195062/diff/14001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

http://codereview.appspot.com/6195062/diff/14001/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:37: #define SH_VERSION 105
This should be incremented since the API is changing.

http://codereview.appspot.com/6195062/diff/14001/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:118: // This flag is only valid if all of the
following are true:
Perhaps instead of saying "is only valid" say "only has an effect" or something
like that.  The current wording makes it seem like it's an error to specify this
for vertex shaders.

http://codereview.appspot.com/6195062/diff/14001/samples/translator/translato...
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/6195062/diff/14001/samples/translator/translato...
samples/translator/translator.cpp:93: compileOptions |= SH_WEB_SAFE;
Would it be useful to have a separate option to toggled the "safeness" check,
instead of assuming that all WebGL shaders will want the check?

http://codereview.appspot.com/6195062/diff/14001/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/6195062/diff/14001/src/build_angle.gyp#newcode125
src/build_angle.gyp:125: 'compiler/websafe/ValidateWebSafeVertexShader.h',
Please add these to src/common/translator_common.vcproj as well (this is used by
the stand-alone VC projects)

http://codereview.appspot.com/6195062/diff/14001/src/compiler/websafe/Depende...
File src/compiler/websafe/DependencyGraph.cpp (right):

http://codereview.appspot.com/6195062/diff/14001/src/compiler/websafe/Depende...
src/compiler/websafe/DependencyGraph.cpp:2: // Copyright (c) 2002-2012 The ANGLE
Project Authors. All rights reserved.
Just use 2012 for the copyright year on the new files.

[kbr1, 2012-05-22 22:47:15]

http://codereview.appspot.com/6195062/diff/14001/AUTHORS
File AUTHORS (right):

http://codereview.appspot.com/6195062/diff/14001/AUTHORS#newcode16
AUTHORS:16: Adobe Systems Inc.
On 2012/05/22 17:57:49, dgkoch wrote:
> I'm not sure Adobe should be added to the AUTHORS file, as we haven't done
this
> for other organizations (but it definitely should to the CONTRIBUTORS).
> 
> Ken?

I'm not a lawyer -- but I agree with this assessment. Mozilla and other
corporations have made substantial contributions to ANGLE and have been added to
the contributors list, not the authors list.

http://codereview.appspot.com/6195062/diff/14001/samples/translator/translato...
File samples/translator/translator.cpp (right):

http://codereview.appspot.com/6195062/diff/14001/samples/translator/translato...
samples/translator/translator.cpp:93: compileOptions |= SH_WEB_SAFE;
On 2012/05/22 17:57:49, dgkoch wrote:
> Would it be useful to have a separate option to toggled the "safeness" check,
> instead of assuming that all WebGL shaders will want the check?

I think that would be a good idea.

[mvujovic, 2012-05-23 00:02:18]

Thanks for the review, Daniel! I've updated the patch with the changes and
responded inline to your comments and Ken's.

On 2012/05/22 17:57:49, dgkoch wrote:
> I definitely like the name SH_TIMING_RESTRICTIONS better than SH_WEBGL_SAFE.
> Seems like you should also rename a number of the classes and function as well
> to reflect that name.

Sounds good. I've updated all mentions of "web safe" to "timing restrictions".

> Another suggestion about new files: Right now they are all in a "websafe"
> folder.  I'd suggest putting all the dependency graph related ones into a
> separate folder "depgen" or something, and then another one for the webgl
> specific validations.

Good idea. I've put the dependency graph related files in a "depgraph" folder,
and the timing restrictions related files in a "timing" folder.

On 2012/05/22 22:47:15, kbr1 wrote:
> http://codereview.appspot.com/6195062/diff/14001/AUTHORS
> File AUTHORS (right):
>
> http://codereview.appspot.com/6195062/diff/14001/AUTHORS#newcode16
> AUTHORS:16: Adobe Systems Inc.
> On 2012/05/22 17:57:49, dgkoch wrote:
> > I'm not sure Adobe should be added to the AUTHORS file, as we haven't done
> this
> > for other organizations (but it definitely should to the CONTRIBUTORS).
> >
> > Ken?
>
> I'm not a lawyer -- but I agree with this assessment. Mozilla and other
> corporations have made substantial contributions to ANGLE and have been added
to
> the contributors list, not the authors list.

Sounds good. I've removed Adobe from the AUTHORS file. Adobe is still in
CONTRIBUTORS file.

> http://codereview.appspot.com/6195062/diff/14001/include/GLSLANG/ShaderLang.h
> File include/GLSLANG/ShaderLang.h (right):
>
>
http://codereview.appspot.com/6195062/diff/14001/include/GLSLANG/ShaderLang.h...
> include/GLSLANG/ShaderLang.h:37: #define SH_VERSION 105
> This should be incremented since the API is changing.

Done.

>
http://codereview.appspot.com/6195062/diff/14001/include/GLSLANG/ShaderLang.h...
> include/GLSLANG/ShaderLang.h:118: // This flag is only valid if all of the
> following are true:
> Perhaps instead of saying "is only valid" say "only has an effect" or
something
> like that.  The current wording makes it seem like it's an error to specify
this
> for vertex shaders.

Agreed. I changed it to say "only has an effect".

>
http://codereview.appspot.com/6195062/diff/14001/samples/translator/translato...>
File samples/translator/translator.cpp (right):
>
>
http://codereview.appspot.com/6195062/diff/14001/samples/translator/translato...
> samples/translator/translator.cpp:93: compileOptions |= SH_WEB_SAFE;
> On 2012/05/22 17:57:49, dgkoch wrote:
> > Would it be useful to have a separate option to toggled the "safeness"
check,
> > instead of assuming that all WebGL shaders will want the check?
>
> I think that would be a good idea.

Agreed, I think the spec should be toggled separately from the compile flags.
I've added the following flags to translator.cpp:

-s=e  : use GLES2 spec (this is by default)
-s=w  : use WebGL spec

I've also changed "-w" for "web safe" to "-t" for "timing restrictions".

> http://codereview.appspot.com/6195062/diff/14001/src/build_angle.gyp
> File src/build_angle.gyp (right):
>
>
http://codereview.appspot.com/6195062/diff/14001/src/build_angle.gyp#newcode125
> src/build_angle.gyp:125: 'compiler/websafe/ValidateWebSafeVertexShader.h',
> Please add these to src/common/translator_common.vcproj as well (this is used
by
> the stand-alone VC projects)

Done.

>
http://codereview.appspot.com/6195062/diff/14001/src/compiler/websafe/Depende...
> File src/compiler/websafe/DependencyGraph.cpp (right):
>
>
http://codereview.appspot.com/6195062/diff/14001/src/compiler/websafe/Depende...
> src/compiler/websafe/DependencyGraph.cpp:2: // Copyright (c) 2002-2012 The
ANGLE
> Project Authors. All rights reserved.
> Just use 2012 for the copyright year on the new files.

Done.

[mvujovic, 2012-05-29 20:46:40]

Thank you for the reviews, everyone.

Do you guys think this is ready to land? (after a rebase)

- Max

[kbr1, 2012-05-29 23:04:05]

From my standpoint this looks great -- yes, I'd say land it now. LGTM. Perhaps
Nicolas or Daniel can grant you write access to the repository so you can commit
it yourself. One comment.

https://codereview.appspot.com/6195062/diff/16002/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

https://codereview.appspot.com/6195062/diff/16002/src/compiler/Compiler.cpp#n...
src/compiler/Compiler.cpp:168: // FIXME(mvujovic): For now, we only consider
"u_texture" to be a potentially unsafe symbol.
That's a pretty big FIXME. Could you file a follow-on bug in the ANGLE issue
tracker about it and assign it to yourself? I think it'll probably be necessary
to enforce the timing restrictions against all samplers in the shader.

[mvujovic, 2012-05-29 23:32:34]

On 2012/05/29 23:04:05, kbr1 wrote:
> From my standpoint this looks great -- yes, I'd say land it now. LGTM. Perhaps
> Nicolas or Daniel can grant you write access to the repository so you can
commit
> it yourself. One comment.

Sounds great. I can do that.

> https://codereview.appspot.com/6195062/diff/16002/src/compiler/Compiler.cpp
> File src/compiler/Compiler.cpp (right):
> 
>
https://codereview.appspot.com/6195062/diff/16002/src/compiler/Compiler.cpp#n...
> src/compiler/Compiler.cpp:168: // FIXME(mvujovic): For now, we only consider
> "u_texture" to be a potentially unsafe symbol.
> That's a pretty big FIXME. Could you file a follow-on bug in the ANGLE issue
> tracker about it and assign it to yourself? I think it'll probably be
necessary
> to enforce the timing restrictions against all samplers in the shader.

Sure. I can fix this in a follow up patch. I've made a new issue:
http://code.google.com/p/angleproject/issues/detail?id=332

To assign it to myself, I think I need someone to give me permission to edit
issues.

Thanks,
Max

[kbr1, 2012-05-29 23:35:43]

On 2012/05/29 23:32:34, mvujovic wrote:
> Sure. I can fix this in a follow up patch. I've made a new issue:
> http://code.google.com/p/angleproject/issues/detail?id=332
> 
> To assign it to myself, I think I need someone to give me permission to edit
> issues.

Nicolas, Daniel or Vangelis should be able to help you with this. I think once
you are made a Committer you should be able to edit issues but I'm not 100%
sure.

[dgkoch, 2012-05-30 00:15:17]

LGTM as well.  I've added you to ANGLE as a committer now.

https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
File src/compiler/timing/RestrictFragmentShaderTiming.cpp (right):

https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
src/compiler/timing/RestrictFragmentShaderTiming.cpp:52: if
(parameter->getIntermFunctionCall()->getName() != "texture2D(s21;vf2;" ||
Is this intentional or is this debugging code?

[mvujovic, 2012-05-30 16:55:35]

On 2012/05/30 00:15:17, dgkoch wrote:
> LGTM as well.  I've added you to ANGLE as a committer now.

Great! Thanks, Daniel. I'll commit it today

>
https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
> File src/compiler/timing/RestrictFragmentShaderTiming.cpp (right):
> 
>
https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
> src/compiler/timing/RestrictFragmentShaderTiming.cpp:52: if
> (parameter->getIntermFunctionCall()->getName() != "texture2D(s21;vf2;" ||
> Is this intentional or is this debugging code?

This code is intentional. It demonstrates another type of restriction besides
control flow. It enforces that an author does not use a texture derived value as
the texture coordinate of a texture2D sampling operation. 

We were thinking that someone could mount a timing attack using texture cache
hits or misses (though it hasn't been proven yet). 

In the context of CSS Shaders, we only had to worry about texture2D because the
DOM element texture was a sampler2D. For WebGL, I think we have to worry about
all of the sampling operations like textureCube, etc. 

I can add a FIXME here and create an issue for this concern in case we decide to
enforce this kind of restriction. Does that sound okay?

[dgkoch, 2012-05-30 20:22:44]

On 2012/05/30 16:55:35, mvujovic wrote:
> On 2012/05/30 00:15:17, dgkoch wrote:
> > LGTM as well.  I've added you to ANGLE as a committer now.
> 
> Great! Thanks, Daniel. I'll commit it today
> 
> >
>
https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
> > File src/compiler/timing/RestrictFragmentShaderTiming.cpp (right):
> > 
> >
>
https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
> > src/compiler/timing/RestrictFragmentShaderTiming.cpp:52: if
> > (parameter->getIntermFunctionCall()->getName() != "texture2D(s21;vf2;" ||
> > Is this intentional or is this debugging code?
> 
> This code is intentional. It demonstrates another type of restriction besides
> control flow. It enforces that an author does not use a texture derived value
as
> the texture coordinate of a texture2D sampling operation. 
> 
> We were thinking that someone could mount a timing attack using texture cache
> hits or misses (though it hasn't been proven yet). 
> 
> In the context of CSS Shaders, we only had to worry about texture2D because
the
> DOM element texture was a sampler2D. For WebGL, I think we have to worry about
> all of the sampling operations like textureCube, etc. 
> 
> I can add a FIXME here and create an issue for this concern in case we decide
to
> enforce this kind of restriction. Does that sound okay?
Sure that's fine.

[mvujovic, 2012-05-30 22:45:47]

Committed this patch in r1101.

I wanted to thank you guys for maintaining such a well-architected and readable
codebase. It's been really great working with it so far :)

On 2012/05/30 20:22:44, dgkoch wrote:
> On 2012/05/30 16:55:35, mvujovic wrote:
> > On 2012/05/30 00:15:17, dgkoch wrote:
> > > LGTM as well.  I've added you to ANGLE as a committer now.
> > 
> > Great! Thanks, Daniel. I'll commit it today
> > >
> >
>
https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
> > > File src/compiler/timing/RestrictFragmentShaderTiming.cpp (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/6195062/diff/16002/src/compiler/timing/Restric...
> > > src/compiler/timing/RestrictFragmentShaderTiming.cpp:52: if
> > > (parameter->getIntermFunctionCall()->getName() != "texture2D(s21;vf2;" ||
> > > Is this intentional or is this debugging code?
> > 
> > This code is intentional. It demonstrates another type of restriction
besides
> > control flow. It enforces that an author does not use a texture derived
value
> as
> > the texture coordinate of a texture2D sampling operation. 
> > 
> > We were thinking that someone could mount a timing attack using texture
cache
> > hits or misses (though it hasn't been proven yet). 
> > 
> > In the context of CSS Shaders, we only had to worry about texture2D because
> the
> > DOM element texture was a sampler2D. For WebGL, I think we have to worry
about
> > all of the sampling operations like textureCube, etc. 
> > 
> > I can add a FIXME here and create an issue for this concern in case we
decide
> to
> > enforce this kind of restriction. Does that sound okay?
> Sure that's fine.

I've added issue 335 to cover this.
http://code.google.com/p/angleproject/issues/detail?id=335