I a not sure I placed the code to the best files for the purpose. Glad to hear how I can help more with refactoring.
Description: Refactor the process of choosing validators.
On startup make a choice of validator implementation: write the choice as function pointers in NaClApp. This opens a possibility to add more experimental validators (such as MIPS) without having to add platform-specific stub implementations.
BUG=none TEST=functionality must be left unchanged
Affected files: M src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp M src/trusted/service_runtime/sel_ldr.h M src/trusted/service_runtime/sel_ldr.c M src/trusted/service_runtime/sel_main.c M src/trusted/service_runtime/sel_main_chrome.c M src/trusted/service_runtime/sel_validate_image.c M src/trusted/validator/ncvalidate.h A src/trusted/validator/validation_status.h M src/trusted/validator_arm/ncvalidate.cc
Updated the BUG= to point to the issue I've been using to track the
refactoring.
Welcome to the club.
My comments are mostly about meshing what I've previously thought about with
what you're doing - so there's a lot of mushy theory. As I note below, the
seperating the refactoring from the standalone change would help the review
process significantly - a pain, but please do it.
https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted... src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:41:
['nacl_standalone==0 and target_arch=="ia32"', {
I think you should split the "standalone" change from the bigger
refactoring. It's not necessary to squish them together. The
standalone change should be done ASAP, the refactoring we can polish
until it shines.
I hope you're using git, it makes splitting CLs less of a pain in the
ass. :)
https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted... src/trusted/service_runtime/sel_ldr.h:105: typedef NaClValidationStatus
(*NaClValidateFunc) (
This is part of the validator interface, not part of sel_ldr. My
instinct is that these function definitions (and the validation status
enums) should be in src/validator/ncvalidate.h. The new interface will
eventually replace the old, no reason to add a new file.
https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted... src/trusted/service_runtime/sel_ldr.h:398: NaClValidateFunc
validate_func;
I was envisioning these functions in a separate (but embedded) structure
that also contained the bundle_size parameter. (bundle_size can be
dealt with later, but it is fundamentally a property of the validation
model) This would allow each validator to declare a
InitValidatorInterface(...) function to fill out this structure. Less
coupling.
https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted... src/trusted/service_runtime/sel_validate_image.c:30: static
NaClValidationStatus ValidatorCopyNotImplemented(
I think that each validator that doesn't implement a function should
explicitly provide these stubs itself. sel_ldr selects a validator, and
then the validator fills out the function table - sel_ldr doesn't need
to know the implemented / unimplemented details.
You could have a .c file in src/trusted/validator containing these
stubs, but linking would be easier if each validator declared their own
stub. (It would also be better documentation and provide a skeleton
function to fill out? I realize this may seem to contradict my prior
"no stub for ARM" advice, but that was about adding a new entry point,
not about stubbing out the existing entry points.)
https://chromiumcodereview.appspot.com/10134056/diff/2001/src/trusted... src/trusted/service_runtime/sel_validate_image.c:59: nap->validate_func
= NACL_SUBARCH_NAME(ApplyValidator,
I'd reorganize this into mutually exclusive ifdefs that call
validator-specific functions to fill out the function table. Figure
that this is going to be expanded to deal with x86-32 and x86-64
seperately? In some ways it seems like a step back, but if done right
it should make what validator is being used explicit.
I think (maybe not in this CL) that this function should be moved into
seperate file (in src/trusted/validator?) so that it can include header
files for different validators without poluting sel_ldr's namespace,
etc.
Ah, sorry, I misunderstood what you were doing the the patchsets. I
assumed the
CL you were pointing me to was the CL to include ragel, not a new CL to do
the
standalone change (I am still crawling though my mailbox and didn't look to
carefully.) Forget I said anything about splitting the CL. Me idiot.
If you're using git:
git cl upload a_different_branch will upload a diff between the branch
you're
working on and the branch you specify. You'll need to specify the other
branch
every upload, however, or else if will automatically diff against git-svn.
> Ah, sorry, I misunderstood what you were doing the the patchsets. I
> assumed
the
> CL you were pointing me to was the CL to include ragel, not a new CL to
> do the
> standalone change (I am still crawling though my mailbox and didn't look
> to
> carefully.) Forget I said anything about splitting the CL. Me idiot.
> If you're using git:
> git cl upload a_different_branch will upload a diff between the branch
> you're
> working on and the branch you specify. You'll need to specify the other
branch
> every upload, however, or else if will automatically diff against git-svn.
Thanks! I tried various options and they did not work. Nice to know it's a
mere
argument that sets the base.
> This is part of the validator interface, not part of sel_ldr. My
instinct is
> that these function definitions (and the validation status enums)
should be in
> src/validator/ncvalidate.h. The new interface will eventually replace
the old,
> no reason to add a new file.
> I was envisioning these functions in a separate (but embedded)
structure that
> also contained the bundle_size parameter. (bundle_size can be dealt
with later,
> but it is fundamentally a property of the validation model) This would
allow
> each validator to declare a InitValidatorInterface(...) function to
fill out
> this structure. Less coupling.
well, I thought about this same possibility at first, but then on
callsites:
nap->validate_func(...) is less clutter than:
nap->validator_interface->validate_func(...)
and then I'm not pleased
Less coupling is good when we have use cases for using components
separately or abstractions that are very natural (and named
intuitively). struct ValidatorInterface does not sound as a positive
name for reader's intuition.
Generally I do not mind this decoupling, but for that please give advice
on naming the struct and fields.
> I think that each validator that doesn't implement a function should
explicitly
> provide these stubs itself. sel_ldr selects a validator, and then the
validator
> fills out the function table - sel_ldr doesn't need to know the
implemented /
> unimplemented details.
hm, we seem to agreed that validator selection is a tricky process
involving a lot of parameters: environment vars, platform (maybe OS?),
NaClApp, etc. And it is hard to regularize this process. So, if we leave
this choice partially to each of the "validators", then we risk
separating this process into parts non-trivially which would only
increase complication.
Imagine the need to check for the same env var in many files
identically. Or having to pass many args to InitializeValidator(...)
which would take more lines than a simple choice in a single selection
function.
If the function grows beyond 50 lines of code, I am more open to
reconsidering it's structure and separation. Decoupling too early is
like premature optimization.
> You could have a .c file in src/trusted/validator containing these
stubs, but
> linking would be easier if each validator declared their own stub.
(It would
> also be better documentation and provide a skeleton function to fill
out? I
> realize this may seem to contradict my prior "no stub for ARM" advice,
but that
> was about adding a new entry point, not about stubbing out the
existing entry
> points.)
Moving stubs to a separate utility file sounds good. I'll postpone it
for a later iteration of this review. Filename suggestions wanted.
> I'd reorganize this into mutually exclusive ifdefs that call
validator-specific
> functions to fill out the function table. Figure that this is going
to be
> expanded to deal with x86-32 and x86-64 seperately? In some ways it
seems like
> a step back, but if done right it should make what validator is being
used
> explicit.
That was my first thought as well. Mutually exclusive cases are easy to
read. However, then we get compiler complaints that the stubs are left
unused for some platforms. Then we may decide to make the stubs
non-static, but that pollutes the namespace.
So I can make it non-static mutually exclusive if you express this
preference.
> I think (maybe not in this CL) that this function should be moved into
seperate
> file (in src/trusted/validator?) so that it can include header files
for
> different validators without poluting sel_ldr's namespace, etc.
> #if !defined(__arm__)
> #if defined(NACL_STANDALONE)
> if (getenv("NACL_DANGEROUS_USE_DFA_VALIDATOR") != NULL) {
> NACL_SUBARCH_NAME(InitDfaValidator,
> NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
> } else {
> NACL_SUBARCH_NAME(InitStandardValidator,
> NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
> }
> #else
> NACL_SUBARCH_NAME(InitStandardValidator,
> NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
> #endif
> #else
> NACL_SUBARCH_NAME(InitStandardValidator,
> NACL_TARGET_ARCH, NACL_TARGET_SUBARCH)(&nap->validator);
> #endif
Ouch. So many functions, each of them making a couple of assignments.
Are you sure so much decoupling benefits anyone?
This would lead us to creating quite a few stubs, keeping these stubs
platform-newtral saves some space. And then they could be moved to some
utility file.
Or we could have platform-newtral ValidatorRunner(nap->some_func) that
would return NotImplemented iff nap->some_func == NULL else run the
function
> well, I thought about this same possibility at first, but then on
callsites:
> nap->validate_func(...) is less clutter than:
> nap->validator_interface->validate_func(...)
> and then I'm not pleased
Yeah, the deref chain isn't pretty, but I figured it was just the cost
of doing business. You could always do it in two steps:
> Less coupling is good when we have use cases for using components
separately or
> abstractions that are very natural (and named intuitively). struct
> ValidatorInterface does not sound as a positive name for reader's
intuition.
> Generally I do not mind this decoupling, but for that please give
advice on
> naming the struct and fields.
The type could be NaClValidatorInterface, but the field in NaClApp could
just be "validator".
If these functions were put inside a structure, you could also drop the
_func postfix. validate_copy could also be changed to copy_code - it
doesn't "validate" beyond requiring that it can decode the instructions.
I think there's precedent for function pointers being CamelCase, too.
> hm, we seem to agreed that validator selection is a tricky process
involving a
> lot of parameters: environment vars, platform (maybe OS?), NaClApp,
etc. And it
> is hard to regularize this process. So, if we leave this choice
partially to
> each of the "validators", then we risk separating this process into
parts
> non-trivially which would only increase complication.
As I see it, each "validator" will always behave the same, and will
always have the same interface functions. The only choice is which
validator. Can you describe a specific case where the decision logic
would start to leak into the validator or the interface initialization?
I'll take a shot at figuring out how it could be cleaned up.
> That was my first thought as well. Mutually exclusive cases are easy
to read.
> However, then we get compiler complaints that the stubs are left
unused for some
> platforms. Then we may decide to make the stubs non-static, but that
pollutes
> the namespace.
But if you require each validator declare its own stubs, no compiler
problem. :)
> Ouch. So many functions, each of them making a couple of assignments.
Are you
> sure so much decoupling benefits anyone?
Each of those functions will actually be a single assignment:
Or something like that. Declarative is good. (I am not sure if I like
this idea, but each validator could just declare a const global
interface that NaClApp could point to.) If this method is used,
modifications to a given validator will not require changes to the
service runtime, except if the name of the init function changes.
Keeping the initialization in a function keeps the function table
consistent if it's specified multiple times: see standalone vs. non
standalone. This scheme would also allow us to write generic tests that
iterate over different validators, but that's just a dream right now.
I think, ultimately, I am pushing this design because it allows the
validator to declare its interface. Declarative is the idea that
motivates me.
BTW, that code snippet was ugly, let me take it to its logical
conclusion:
#if defined(__i386__) && defined(NACL_STANDALONE)
if (UseDFAValidator()) {
NaCl_DFAValidatorInit_x86_32(&nap->validator);
} else {
NaCl_ValidatorInit_x86_32(&nap->validator);
}
> On 2012/04/25 20:57:42, Nick Bray wrote:
> > See previous comments, probably want to keep.
> This would lead us to creating quite a few stubs, keeping these stubs
> platform-newtral saves some space. And then they could be moved to
some utility
> file.
> Or we could have platform-newtral ValidatorRunner(nap->some_func) that
would
> return NotImplemented iff nap->some_func == NULL else run the function
Yeah, I was thinking of putting a NULL check in sel_ldr, but I think the
stub function is cleaner. (I think this is a pattern. Active Sentinel?
Can't find a reference to it.) I do think having each validator
declare its own stubs will be cleaner in the long run, because of
build/link mess, but this isn't a big issue for me - your call.
> > hm, we seem to agreed that validator selection is a tricky process
involving a
> > lot of parameters: environment vars, platform (maybe OS?), NaClApp,
etc. And
> it
> > is hard to regularize this process. So, if we leave this choice
partially to
> > each of the "validators", then we risk separating this process into
parts
> > non-trivially which would only increase complication.
> As I see it, each "validator" will always behave the same, and will
always have
> the same interface functions. The only choice is which validator.
Can you
> describe a specific case where the decision logic would start to leak
into the
> validator or the interface initialization?
Purely hypothetical: if we want to choose validator strategy via nacl
manifest. Less hypothetical: we'd want to patch TLS access sequences on
some OS/Machine configurations, but not on others (for speedup), where
platform qualification may be involved.
Okay, I made a draft change with full control over the choice of
validation to the platform-dependent files. I could not separate
not-implemented stubs into a separate lib because the build system won
the battle. As a result there is much more boilerplate code now: stubs,
function declarations, trivial implementations repeated all over. Though
anyway, looks better than it was.
I uploaded a draft change. Please, look at the general idea, do not pay
attention to various extra TODOs left there temporarily. Enjoy code
duplication :)
Also, I could not make it to run the validation_cache_test, getting
undefined reference to 'NaCl_mprotect', no idea why and how I could
affect that.
Also, one funny/annoying thing, this stuff gets built even if I ask to
only build platform=x86-32:
scons-out/dbg-linux-x86-32/obj/src/trusted/validator_arm/libncvalidate_arm_ v2.a
> Purely hypothetical: if we want to choose validator strategy via nacl
manifest.
That would affect the choice of validator and would not leak into any
particular validator. What we're doing would support this cleanly.
> Less hypothetical: we'd want to patch TLS access sequences on some
OS/Machine
> configurations, but not on others (for speedup), where platform
qualification
> may be involved.
Hmmm.
1) Add a "patch_TLS" argument to the validator in question.
2) Create two new entry points that wrap this validator, one passing
"1", the other passing "0".
3) Either treat the two new entry points as "different validators", or
parameterize the interface construction function.
Alternatively, this was suggested by bsy, we could move from a table of
pointers to a "object oriented" approach where there was data associated
with each validator. I rather not make that change until we need to,
however.
> Okay, I made a draft change with full control over the choice of
validation to
> the platform-dependent files. I could not separate not-implemented
stubs into a
> separate lib because the build system won the battle. As a result
there is much
> more boilerplate code now: stubs, function declarations, trivial
implementations
> repeated all over. Though anyway, looks better than it was.
I take no pleasure that the stubs got duplicated, I just know not to bet
against the build system.
> Also, I could not make it to run the validation_cache_test, getting
undefined
> reference to 'NaCl_mprotect', no idea why and how I could affect that.
I suspect that we're relying on accidental dependencies, somewhere,
somehow?
> Also, one funny/annoying thing, this stuff gets built even if I ask to
only
> build platform=x86-32:
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/truste... src/trusted/service_runtime/sel_validate_image.c:38: /* TODO: make it
more nested. */
I started with it being nested, but unless you indent the preprocessor
directives, it looks horrible. Hence the linear structure. :/
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/truste... src/trusted/service_runtime/sel_validate_image.c:168: return
NaClValidateStatus(nap->validator->ValidateCopy(
Bad name. Validating a copy of what? Hence why I suggested "CopyCode".
I understand why you want to have all the function have the same
prefix, but I think it confuses what the verb is supposed to be. (Copy,
not Validate.)
https://chromiumcodereview.appspot.com/10134056/diff/15001/src/truste... src/trusted/validator/ncvalidate.h:126: NaClValidateCopyFunc
ValidateCopy;
Nit: swap position of Copy and Replacement? (Copy is less important
that the replacement function.) Veeeery nitish. it bothers me, but I
realize I am being way too picky. Please ignore me.
> ValidatorCopyNotImplemented(
> > Purely hypothetical: if we want to choose validator strategy via nacl
> manifest.
> That would affect the choice of validator and would not leak into any
particular
> validator. What we're doing would support this cleanly.
I meant that the validator choice should remain the same, while the
validator
itself behaves differently. The second example appears to discuss exactly
this.
> > Less hypothetical: we'd want to patch TLS access sequences on some
OS/Machine
> > configurations, but not on others (for speedup), where platform
qualification
> > may be involved.
> Hmmm.
> 1) Add a "patch_TLS" argument to the validator in question.
> 2) Create two new entry points that wrap this validator, one passing "1",
> the
> other passing "0".
> 3) Either treat the two new entry points as "different validators", or
> parameterize the interface construction function.
> Alternatively, this was suggested by bsy, we could move from a table of
pointers
> to a "object oriented" approach where there was data associated with each
> validator. I rather not make that change until we need to, however.
So you see, this design does not fit very all validator choice strategies
that
we envision. And that is good (i.e. less code). Does it fit very well with
the
current validator choice? Maybe, but the same thing could be expressed in
less
files, less functions, in a more compact and less modular way. The choice
is not
trivial.
> > Okay, I made a draft change with full control over the choice of
> validation
to
> > the platform-dependent files. I could not separate not-implemented stubs
into
> a
> > separate lib because the build system won the battle. As a result there
> is
> much
> > more boilerplate code now: stubs, function declarations, trivial
> implementations
> > repeated all over. Though anyway, looks better than it was.
> I take no pleasure that the stubs got duplicated, I just know not to bet
against
> the build system.
:)
> > Also, I could not make it to run the validation_cache_test, getting
undefined
> > reference to 'NaCl_mprotect', no idea why and how I could affect that.
> I suspect that we're relying on accidental dependencies, somewhere,
> somehow?
unfortunately, I did not have time today to investigate
> > Also, one funny/annoying thing, this stuff gets built even if I ask to
> only
> > build platform=x86-32:
> src/trusted/service_runtime/sel_validate_image.c:38: /* TODO: make it more
> nested. */
> I started with it being nested, but unless you indent the preprocessor
> directives, it looks horrible. Hence the linear structure. :/
I saw indenting preprocessor directives in some projects. Beautiful. Then I
thought it's okay.
> src/trusted/service_runtime/sel_validate_image.c:168: return
> NaClValidateStatus(nap->validator->ValidateCopy(
> Bad name. Validating a copy of what? Hence why I suggested "CopyCode". > I
> understand why you want to have all the function have the same prefix,
> but I
> think it confuses what the verb is supposed to be. (Copy, not Validate.)
I was making these names mechanically, did not think how they sound. Will
rename.
> src/trusted/validator/ncvalidate.h:126: NaClValidateCopyFunc ValidateCopy;
> Nit: swap position of Copy and Replacement? (Copy is less important that
> the
> replacement function.) Veeeery nitish. it bothers me, but I realize I am
being
> way too picky. Please ignore me.
> src/trusted/validator/x86/64/ncvalidate.c:143: void
> NaClValidatorInit_x86_64(struct NaClValidatorInterface **val) {
> Maybe this should be a getter (returns the pointer) instead of a setter?
> src/trusted/validator_arm/ncvalidate.cc:48: static NaClValidationStatus
> ValidatorCopyNotImplemented(
> For consistency with ragel, place these functions next to the interface
> declaration.
Why do you say 'consistency with ragel'? I saw BradC writing 'I added
support for ragel in tests'. Is it a kind of irony here? We are not changing
ragel, only operate on something ragel-based, where ragel is just a tool.
In the
same
way we could say 'add support for python in tests' meaning that service
runtime
is
built with python code on buildbots.
If it were not too long of a name, I would have referred to it
as 'ragel-based
validator' or 'ragel-generated dfa parsing code'. Instead I name it as
'validator_ragel'. Less ambiguous.
