I should start by saying that I've not actually used Safe yet
- but I'd like to soon...
First off I'd like to congratulate Malcolm (and Larry) on an elegant
implementation. So much power in such a small amount of code.
*** ext/Safe/Safe.xs.ORI Fri Dec 15 17:35:48 1995
--- ext/Safe/Safe.xs Fri Dec 15 16:43:51 1995
+
+ void
+ safe_call_sv(package, mask, codesv)
+ char * package
+ SV * mask
+ SV * codesv
+ CODE:
+ int i;
+ char *str;
+ STRLEN len;
+
+ ENTER;
+ SAVETMPS;
+ save_hptr(&defstash);
+ save_aptr(&endav);
+ SAVEPPTR(op_mask);
+ Newz(666, op_mask, maxo, char);
+ SAVEFREEPV(op_mask);
+ str = SvPV(mask, len);
+ for (i = 0; i < maxo && i < len; i++)
+ op_mask[i] = str[i];
Fatal error if len != maxo?
--- ext/Safe/Safe.pm Fri Dec 15 16:53:44 1995
+ =item a new namespace
+ By default, the only variables shared with
+ compartments are $_ and @_.
Since it's a glob assignment it might be worth mentioning
the '_' filehandle.
+ =item an operator mask
+ This includes masking off operators such as I<system>, I<open>,
+ I<chown>, and I<shmget> but does not mask off operators such as
+ I<print>, I<sysread> and I<E<lt>HANDLEE<gt>>.
Typo?
+ =head2 Operator masks
+
+ An operator mask exists at user-level as a string of bytes of length
+ MAXO, each of which is either 0x00 or 0x01. Here, MAXO is the number
+ of operators in the current version of perl. The subroutine MAXO()
+ (available for export by package Safe) returns the number of operators
+ in the version of perl at the time the Safe extension was B<built>.
+
+ If the Safe module is used as a dynamic extension then it should not
+ be used with a future version of perl which has a different number
+ of operators.
That is a *cricial* issue.
+ The presence of a 0x01 byte at offset B<n> of the string
+ indicates that operator number B<n> should be masked (i.e. disallowed).
+ The Safe extension makes available routines for converting from operator
+ names to operator numbers (and I<vice versa>) and for converting from a
+ list of operator names to the corresponding mask (and I<vice versa>).
I suggest that you do all the following:
- Remove or hide all support for opcode numbers. If you can't remove it
I'd suggest turning them into 'objects' (blessed ref to IV) in order
to validate that the 'number' has been obtained by looking up it's
value by name within this process and not simply hard coded by someone
trying to avoid the lookup cost in the name of efficiency.
- All operator masks should be checked for being exactly MAXO long.
- Force Safe to only work with the perl version it was built for (check
compile-time PATCHLEVEL against run-time $]).
Given the nature of the Safe module (and the fact that it should be
supplied and rebuilt with perl) I'd *much* rather it was very strict
about this than run the risk of opcode number mismatches.
+ is the root namespace to use for the compartment (defaults to
+ "Safe::Root000000000", auto-incremented for each new compartment); and
Auto-inc isn't very cheap. Why not just use a counter 1,2,3... etc?
Warn about memory leakage from undestroyed packages.
+ my $safes = "1111111111111111111111101111111111111111111111111111111111111111"
+ . "1111111111111111111111111111111111111111111111111111111111111111"
+ . "1111110011111111111011111111111111111111111111111111111101001010"
+ . "0110111111111111111111110011111111100001000000000000000000000100"
+ . "0000000000000111110000001111111110100000000000001111111111111111"
+ . "11111111111111111110";
Eeek!
I really hope that came from a script that processes the opcode lists.
As it stands now any version-mismatch checking code you add will be
totally defeated by this hard-coded list.
Ideally a 'safe' flag should be added to opcode.pl. Failing that (or
in the meantime) a file of safe opcode _names_ could be supplied with
the extension and processed into the opmask at build time. Parsing
opcode.h from $archlib/CORE would not be too hard (especially if
opcode.pl was tweaked to add numbers after the textual names).
+ sub reval {
+ my ($obj, $expr) = @_;
+ my $root = $obj->{Root};
+ my $mask = $obj->{Mask};
+ verify_mask($mask);
+
+ my $evalsub = eval sprintf(<<'EOT', $root);
+ package %s;
+ sub {
+ eval $expr;
+ }
+ EOT
+ return safe_call_sv($root, $mask, $evalsub);
+ }
This seems rather expensive. Why not do $expr = "package $root; $expr";
and avoid the double eval? Now that Larry's added perl_eval_sv() the
whole method could be moved to XS for speed.
I *really* want to see Safe included in the standard distribution and,
even with the comments above, I'd be very happy to see this release
included now (partly to raise the profile of it).
Personally I would not want to recommend it for production use until
some or all of these issues have been addressed.
Tim.
- but I'd like to soon...
First off I'd like to congratulate Malcolm (and Larry) on an elegant
implementation. So much power in such a small amount of code.
*** ext/Safe/Safe.xs.ORI Fri Dec 15 17:35:48 1995
--- ext/Safe/Safe.xs Fri Dec 15 16:43:51 1995
+
+ void
+ safe_call_sv(package, mask, codesv)
+ char * package
+ SV * mask
+ SV * codesv
+ CODE:
+ int i;
+ char *str;
+ STRLEN len;
+
+ ENTER;
+ SAVETMPS;
+ save_hptr(&defstash);
+ save_aptr(&endav);
+ SAVEPPTR(op_mask);
+ Newz(666, op_mask, maxo, char);
+ SAVEFREEPV(op_mask);
+ str = SvPV(mask, len);
+ for (i = 0; i < maxo && i < len; i++)
+ op_mask[i] = str[i];
Fatal error if len != maxo?
--- ext/Safe/Safe.pm Fri Dec 15 16:53:44 1995
+ =item a new namespace
+ By default, the only variables shared with
+ compartments are $_ and @_.
Since it's a glob assignment it might be worth mentioning
the '_' filehandle.
+ =item an operator mask
+ This includes masking off operators such as I<system>, I<open>,
+ I<chown>, and I<shmget> but does not mask off operators such as
+ I<print>, I<sysread> and I<E<lt>HANDLEE<gt>>.
Typo?
+ =head2 Operator masks
+
+ An operator mask exists at user-level as a string of bytes of length
+ MAXO, each of which is either 0x00 or 0x01. Here, MAXO is the number
+ of operators in the current version of perl. The subroutine MAXO()
+ (available for export by package Safe) returns the number of operators
+ in the version of perl at the time the Safe extension was B<built>.
+
+ If the Safe module is used as a dynamic extension then it should not
+ be used with a future version of perl which has a different number
+ of operators.
That is a *cricial* issue.
+ The presence of a 0x01 byte at offset B<n> of the string
+ indicates that operator number B<n> should be masked (i.e. disallowed).
+ The Safe extension makes available routines for converting from operator
+ names to operator numbers (and I<vice versa>) and for converting from a
+ list of operator names to the corresponding mask (and I<vice versa>).
I suggest that you do all the following:
- Remove or hide all support for opcode numbers. If you can't remove it
I'd suggest turning them into 'objects' (blessed ref to IV) in order
to validate that the 'number' has been obtained by looking up it's
value by name within this process and not simply hard coded by someone
trying to avoid the lookup cost in the name of efficiency.
- All operator masks should be checked for being exactly MAXO long.
- Force Safe to only work with the perl version it was built for (check
compile-time PATCHLEVEL against run-time $]).
Given the nature of the Safe module (and the fact that it should be
supplied and rebuilt with perl) I'd *much* rather it was very strict
about this than run the risk of opcode number mismatches.
+ is the root namespace to use for the compartment (defaults to
+ "Safe::Root000000000", auto-incremented for each new compartment); and
Auto-inc isn't very cheap. Why not just use a counter 1,2,3... etc?
Warn about memory leakage from undestroyed packages.
+ my $safes = "1111111111111111111111101111111111111111111111111111111111111111"
+ . "1111111111111111111111111111111111111111111111111111111111111111"
+ . "1111110011111111111011111111111111111111111111111111111101001010"
+ . "0110111111111111111111110011111111100001000000000000000000000100"
+ . "0000000000000111110000001111111110100000000000001111111111111111"
+ . "11111111111111111110";
Eeek!
I really hope that came from a script that processes the opcode lists.
As it stands now any version-mismatch checking code you add will be
totally defeated by this hard-coded list.
Ideally a 'safe' flag should be added to opcode.pl. Failing that (or
in the meantime) a file of safe opcode _names_ could be supplied with
the extension and processed into the opmask at build time. Parsing
opcode.h from $archlib/CORE would not be too hard (especially if
opcode.pl was tweaked to add numbers after the textual names).
+ sub reval {
+ my ($obj, $expr) = @_;
+ my $root = $obj->{Root};
+ my $mask = $obj->{Mask};
+ verify_mask($mask);
+
+ my $evalsub = eval sprintf(<<'EOT', $root);
+ package %s;
+ sub {
+ eval $expr;
+ }
+ EOT
+ return safe_call_sv($root, $mask, $evalsub);
+ }
This seems rather expensive. Why not do $expr = "package $root; $expr";
and avoid the double eval? Now that Larry's added perl_eval_sv() the
whole method could be moved to XS for speed.
I *really* want to see Safe included in the standard distribution and,
even with the comments above, I'd be very happy to see this release
included now (partly to raise the profile of it).
Personally I would not want to recommend it for production use until
some or all of these issues have been addressed.
Tim.