Priority: Urgent
Current State: Closed
Released In: 2.0.0
Target Release: major
This was encountered with
PerlPlugin when a PERL type could not be eval'd due to embedded comments.
Create a simple
Config.spec
file with a bad PERL definition. The comment
#'&getUrlHost',
triggers the error.
$Foswiki::cfg{Plugins}{PerlPlugin}{Share} = {
'Foswiki::Func' => [
'&getSkin',
#'&getUrlHost',
'&getScriptUrl',
]
};
Failure occurs in
LoadSpec::parse()
, resulting in:
Foswiki configure
Error
Foswiki::Configure::Query::getspec errors: /var/www/foswiki/distro/core/lib/Foswiki/Plugins/PerlPlugin/Config.spec: 19: Cannot eval value '{ 'Foswiki::Func' => [ '&getSkin', #'&getUrlHost', '&getScriptUrl', ] }': syntax error at (eval 897) line 1, at EOF
Missing right curly or square bracket at (eval 897) line 1, at end of line
at /usr/lib/perl5/vendor_perl/5.18.2/CGI/Carp.pm line 378.
CGI::Carp::realdie('Foswiki::Configure::Query::getspec errors: /var/www/foswiki/d...') called at /usr/lib/perl5/vendor_perl/5.18.2/CGI/Carp.pm line 467
CGI::Carp::die('Foswiki::Configure::Query::getspec errors: /var/www/foswiki/d...') called at /var/www/foswiki/distro/core/lib/Foswiki/Plugins/ConfigurePlugin.pm line 171
Foswiki::Plugins::ConfigurePlugin::__ANON__('Foswiki=HASH(0x883bbf0)', 'Foswiki::Contrib::JsonRpcContrib::Request=HASH(0x8dedf00)', 'Foswiki::Response=HASH(0x883bcc0)', undef) called at /var/www/foswiki/distro/core/lib/Foswiki/Contrib/JsonRpcContrib/Server.pm line 175
Foswiki::Contrib::JsonRpcContrib::Server::__ANON__() called at /usr/lib/perl5/vendor_perl/5.18.2/Error.pm line 419
eval {...} called at /usr/lib/perl5/vendor_perl/5.18.2/Error.pm line 411
Error::subs::try('CODE(0x8fd3408)', 'HASH(0x8fd35b8)') called at /var/www/foswiki/distro/core/lib/Foswiki/Contrib/JsonRpcContrib/Server.pm line 189
Foswiki::Contrib::JsonRpcContrib::Server::dispatch('Foswiki::Contrib::JsonRpcContrib::Server=HASH(0x8e0e710)', 'Foswiki=HASH(0x883bbf0)') called at /var/www/foswiki/distro/core/lib/Foswiki/Contrib/JsonRpcContrib.pm line 48
Foswiki::Contrib::JsonRpcContrib::dispatch('Foswiki=HASH(0x883bbf0)') called at /var/www/foswiki/distro/core/lib/Foswiki/UI.pm line 372
Foswiki::UI::__ANON__() called at /usr/lib/perl5/vendor_perl/5.18.2/Error.pm line 419
eval {...} called at /usr/lib/perl5/vendor_perl/5.18.2/Error.pm line 411
Error::subs::try('CODE(0x804dd38)', 'HASH(0x88133b8)') called at /var/www/foswiki/distro/core/lib/Foswiki/UI.pm line 498
Foswiki::UI::_execute('Foswiki::Request=HASH(0x8822750)', 'CODE(0x8822600)', 'jsonrpc', 1) called at /var/www/foswiki/distro/core/lib/Foswiki/UI.pm line 324
Foswiki::UI::handleRequest('Foswiki::Request=HASH(0x8822750)') called at /var/www/foswiki/distro/core/lib/Foswiki/Engine/CGI.pm line 98
Foswiki::Engine::CGI::run('Foswiki::Engine::CGI=HASH(0x83a6e50)') called
I added print statements around the failing code. $value is set to the invalid string after eval. So the bad value is retained. I tried clearing $value if $@, but the it also fails with an "Incomplete PERL definition. Also tried setting it to an empty array, still fails. Not sure where to go next.
ABOUT TO EVAL { 'Foswiki::Func' => [ '&getSkin', #'&getUrlHost', '&getScriptUrl', ] }
RETURNING { 'Foswiki::Func' => [ '&getSkin', #'&getUrlHost', '&getScriptUrl', ] }
Rather than completely killing configure, requiring file system intervention to fix a bad Config.spec, we should skip the entry, and allow configure to proceed with errors.
--
GeorgeClark - 28 Dec 2014
Previous discussion copied from
Item8572:
--
GeorgeClark - 27 Dec 2014
Completely wipes out 1.2 configure. This needs some fixup before 1.2 release! I can't get into configure with this extension installed.
Points to some issues in the Config.spec parser. It's become more brittle.
-- GeorgeClark - 24 Dec 2014
The issue is that the Config.spec
shipped with PerlPlugin includes some embedded comments that don't eval. We could probably stand to handle this a bit more gracefully in configure. Committed fix to PerlPlugin. This task can "No Action" ... configure 1.2 displays the settings in the right location.
-- GeorgeClark - 26 Dec 2014
I designed configure on the principle that Config.spec's would be tested by extension authors.
We could protect against Config.spec authors shooting themselves in the foot. That would require eval of all DISPLAY_IF and CHECK expressions as well, which requires a browser. I don't know any way to do that automatically.
-- CrawfordCurrie - 27 Dec 2014
In this case there is an eval in getspec that is trapped and passed to reporter as an ERROR. But for some reason it is trapped and reported as a jsonrpc crash and not as an item/value error. In this block in Foswiki::Configure::LoadSpec::parse()
# check it's a valid perl expression, ignoring uninitialised
# variable warnings inside strings.
no warnings;
$value =~ /^(.*)$/;
eval $1;
$reporter->ERROR("$context: Cannot eval value '$value': $@") if $@;
$value = $1;
use warnings;
I tried a local sig die and a few other things but could not prevent the error from crashing jsonrpc. The eval is completely ineffective. I wonder if this is related to the generally buggy / magical aspects of try/catch. If the reporter was actually trapping the error and allowing configure to proceed, we'd be in much better shape.
-- GeorgeClark - 27 Dec 2014
Dug just a little further. On 1.1.9, the comments in the Config.spec perl definition appear to be silently filtered out. I installed PerlPlugin from Extensions web on my 1.1.9 test system to no ill effect. Configure apparently doesn't eval the perl, or if it does, it cleans it first.
I've now thoroughly hijacked this task The original problem was a "no action" though. On 1.1.9 the settings are in the correct location.
The issue for 1.2 is that upgraders using existing extensions are going to have a configure that doesn't load. So we've retroactively added bullets to their gun, and aimed it at their feet for them. The config.spec files were tested on 1.1.x, and worked there. So we've tightened the spec. That's not a bad thing, but it would be nice if we were graceful about it.
-- GeorgeClark - 27 Dec 2014
The only way I can see that would fail is if $1 is wrong, because the regex failed for some reason (e.g. $value was undef)
-- CrawfordCurrie - 27 Dec 2014
This was left as "Waiting for feedback" from me, but I have no idea what feedback I'm meant to provide, so kicking back to George.
--
CrawfordCurrie - 17 Jan 2015
I'm just concerned that we've tightened up the parser, and made it more likely that an existing plugin with a bad
Config.spec
will crash configure and there is no recover other than manual edit. We need to be more forgiving. On some platforms where users don't have shell access, this could be quite disastrous.
--
GeorgeClark - 18 Jan 2015
Okay... debugged this again. it's really difficult because the warning gives not a clue as to where the failure occurs. I still have not found the actual eval that fails. But the bug is that we used to support the following in a spec file:
$Foswiki::cfg{Plugins}{PerlPlugin}{Share} = {
'Foswiki::Func' => [
'&getSkin',
#'&getUrlHost',
'&getScriptUrl',
]
};
Config setting and spec files could contain embedded newlines comments within a perl code segment.
The problem gets caused when the following code in
LoadSpec removes the newlines from settings:
sub _loadSpecsFrom {
@@ -343,8 +344,8 @@ sub parse {
while ( $value !~ s/\s*;\s*$// ) {
my $cont = <$fh>;
last unless defined $cont;
chomp $cont;
$cont =~ s/^\s+/ /;
unless ( $cont =~ /^#/ ) {
$value .= $cont;
}
This runs the entire structure together as a single string. The comment ends up in the middle of the string, and eval crashes. And it's that much worse because I've been unable to find any way to determine where it actually crashes.
Removing the chomp prevents the crash, but the rest of the architecture can't tolerate newlines within perl segments. So even though no crash, the default fails to make it to the UI.
Another issue here is that complex perl structures are pretty close to unreadable when whitespace is removed. Reviewing all of the extensions with PERL types, some of them will be nearly unconfigurable without whitespace.
Take a look at the spec files for
DBIStoreContrib Once all the white space is squashed out, how will mere mortals configure them?
{'FIELD' => {'name' => {'index' => 1,'type' => 'VARCHAR(128)'},'tid' => {'type' => 'INT'},'title' => {'type' => 'VARCHAR(256)'},'value' => {'type' => 'TEXT'}},'FILEATTACHMENT' => {'attr' => {'type' => 'VARCHAR(32)'},'comment' => {'truncate_to' => 512,'type' => 'VARCHAR(512)'},'date' => '_DATE','name' => {'index' => 1,'type' => 'VARCHAR(256)'},'path' => {'type' => 'VARCHAR(256)'},'size' => {'type' => 'VARCHAR(32)'},'tid' => {'type' => 'INT'},'user' => '_USERNAME','version' => {'type' => 'VARCHAR(32)'}},'FORM' => {'name' => {'index' => 1,'type' => 'VARCHAR(256)'},'tid' => {'type' => 'INT'}},'PREFERENCE' => {'name' => {'index' => 1,'type' => 'VARCHAR(64)','unique' => 'onepref'},'tid' => {'type' => 'INT','unique' => 'onepref'},'type' => {'type' => 'VARCHAR(32)'},'value' => '_DEFAULT'},'TOPICINFO' => {'author' => '_USERNAME','comment' => {'type' => 'VARCHAR(512)'},'date' => '_DATE','encoding' => {'type' => 'VARCHAR(32)'},'format' => {'type' => 'VARCHAR(32)'},'reprev' => {'type' => 'VARCHAR(32)'},'rev' => {'type' => 'VARCHAR(32)'},'tid' => {'type' => 'INT','unique' => 'onetopicinfo'},'version' => {'type' => 'VARCHAR(256)'}},'TOPICMOVED' => {'by' => {'type' => 'VARCHAR(256)'},'date' => '_DATE','from' => {'type' => 'VARCHAR(256)'},'tid' => {'type' => 'INT'},'to' => {'type' => 'VARCHAR(256)'}},'TOPICPARENT' => {'name' => {'index' => 1,'type' => 'VARCHAR(256)'},'tid' => {'type' => 'INT','unique' => 'oneparent'}},'_DATE' => {'type' => 'VARCHAR(32)'},'_DEFAULT' => {'type' => 'TEXT'},'_USERNAME' => {'index' => 1,'type' => 'VARCHAR(64)'},'metatypes' => {'name' => {'primary' => 1,'type' => 'VARCHAR(63)'}},'topic' => {'name' => {'index' => 1,'type' => 'VARCHAR(128)','unique' => 'webtopic'},'raw' => '_DEFAULT','text' => '_DEFAULT','tid' => {'primary' => 1,'type' => 'INT'},'web' => {'index' => 1,'type' => 'VARCHAR(256)','unique' => 'webtopic'}}}
vs:
$Foswiki::cfg{Extensions}{DBIStoreContrib}{Schema} = {
_DEFAULT => { type => 'TEXT' },
_USERNAME => { type => 'VARCHAR(64)', index => 1 },
_DATE => { type => 'VARCHAR(32)' },
topic => {
_level => 0,
tid => { type => 'INT', primary => 1 }
web => { type => 'VARCHAR(256)', index => 1 },
name => { type => 'VARCHAR(128)', index => 1 },
text => '_DEFAULT',
raw => '_DEFAULT'
},
metatypes => {
_level => 0,
name => { type => 'VARCHAR(63)', index => 1 },
},
TOPICINFO => {
_level => 1,
author => '_USERNAME',
version => { type => 'VARCHAR(256)' },
date => '_DATE',
format => { type => 'VARCHAR(32)' },
reprev => { type => 'VARCHAR(32)' },
rev => { type => 'VARCHAR(32)' },
comment => { type => 'VARCHAR(512)' },
encoding => { type => 'VARCHAR(32)' },
},
TOPICMOVED => {
_level => 1,
from => { type => 'VARCHAR(256)' },
to => { type => 'VARCHAR(256)' },
by => { type => 'VARCHAR(256)' },
date => '_DATE',
},
TOPICPARENT => {
_level => 1,
name => { type => 'VARCHAR(256)', index => 1 },
},
FILEATTACHMENT => {
_level => 1,
name => { type => 'VARCHAR(256)', index => 1 },
version => { type => 'VARCHAR(32)' },
path => { type => 'VARCHAR(256)' },
size => { type => 'VARCHAR(32)' },
date => '_DATE',
user => '_USERNAME',
comment => { type => 'VARCHAR(512)', truncate_to => 512 },
attr => { type => 'VARCHAR(32)' }
},
FORM => {
_level => 1,
name => { type => 'VARCHAR(256)', index => 1 },
},
Actually I think this one rises to the level of urgent. I can't see
DBIStoreContrib being usable with the way configure scrambles the PERL type fields. It looks like it actually passes them through a real hash and recreates them, which on recent perl, reorders the hash... after every update.
LogDispatchContrib has the same issue as does
LdapGuiContrib, There are 50 extensions that make use of PERL datatypes, though most not to this level of complexity.
--
GeorgeClark - 18 Jan 2015
My intent was to canonicalise and simply the configured values to maximise load speed of the config at "the sharp end". However if losing the format of PERL values is such a big deal, then so be it.
--
CrawfordCurrie - 18 Jan 2015
I don't know for sure. I just can't imagine how someone would practically change values like that DBI Schema. How would you work with that?
--
GeorgeClark - 18 Jan 2015
Re-opening. I think that this was fixed at one point, but fields are again losing their structure in the
LocalSite.cfg. See the
Store -> DataForm Settings: {FormTypes}
.
The field is scrambled and unreadable.
- Press "Reset" - field gains structure, and shows up as modified
- Save Configuration
- Field returns to unstructured on-line.
--
GeorgeClark - 08 Feb 2015
When I try it, the field value accurately reflects what is stored in
LocalSite.cfg. I'm obviously missing something; what steps do I have to follow to reproduce the problem?
--
CrawfordCurrie - 15 Feb 2015
Crawford, after your last patch, the formatting is greatly improved. I'm testing with
Store -> Data Forms -> {FormTypes}
There is an issue with recognizing that the value is at it's default setting, (ie configure renders the "reset" button). But... that behavior is the same, with or without your patch. So there is some underlying issue there regardless.
The difference in the fields:
- Value before reset:
[
{
'multivalued' ...
- Value after reset:
[ {
'multivalued'...
Just an extra newline between the
[
and
{
. Add that newline and indent back in and the "modified" status goes away and the "reset" button returns.
The newline appears to be disappearing between the Foswiki.spec file (which has it) and the DOM default value, which does not.
--
GeorgeClark - 16 Feb 2015
Committed a fix that restores the initial \n for multi-line spec. It still doesn't fully match however. The easier solution is to fix up the spec to match the formatted results, however even with that it's not detecting it as default, so I'm not checking in the spec changes.
--
GeorgeClark - 16 Feb 2015
I think the issue is down in Types.js. Even with
$Foswiki::cfg{FormTypes}
identical in spaces and ordering between
LocalSite.cfg
and
Foswiki.spec
,
types.js
is still saying they are different.
--
GeorgeClark - 16 Feb 2015
Javascript can't parse
PERL
values, the best it can do is a string match.
Foswiki::Configure::Reporter
formats the current value using Data::Dumper. This sorts the keys into alphabetic order, and uses 2-space indentation. If the
.spec
is not also formatted alphabetically and with the same indentation (as was the case), then it won't match.
I have modified the comparison to be a bit more tolerant, but bottom line is you have to sort the formatting in the .spec.
--
CrawfordCurrie - 17 Feb 2015