Item13181: Config.spec PERL type scrambled, and configure crashes if PERL type contains any embedded comments.

pencil
Priority: Urgent
Current State: Closed
Released In: 2.0.0
Target Release: major
Applies To: Engine
Component: Configure, ConfigurePlugin
Branches: master
Reported By: GeorgeClark
Waiting For:
Last Change By: GeorgeClark
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 frown, sad smile 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

 
Topic revision: r13 - 06 Jul 2015, GeorgeClark
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy