On Mon, Apr 29, Josef Reidinger wrote:
> On Mon, 29 Apr 2013 16:30:20 +0200
> Thomas Fehr <[email protected]> wrote:
>
> > Hi all,
> >
> > Last week I was confronted with an L3 from Intel regarding autoyast.
> > After some investigations I found out it was caused by them using
> > "<fstopt config:type="symbol">pri=1</fstopt>"
> > instead of correct
> > "<fstopt>pri=1</fstopt>"
> > in partitioning section of autoyast xml code (since they let autoyast
> > xml code itself been generated by scripts this was not completely
> > easy to see). Ok, there are many ways to break an autoyast
> > installation with wrong xml entries but the effects of above bug was
> > quite surprising. YaST2 scr crashed at end of installation when
> > autoyast writes autoyast settings with SCR::Write( .target.ycp, ...)
> > into installed system. At this point in installation SCR has already
> > been restarted below /mnt and the chrooted SCR gets the ycp code via
> > parsing stdin.
> >
> > Here the problem really starts:
> >
> > Above xml leads to followingq map entry: $["fstopt" : `pri=1], but
> > the YCP parser will not accept this.
> > This leads to chrooted SCR at end of installation to crash which
> > of course wreaks furter havoc afterwards.
> >
> > YCP parser in core/libycp/src/scanner.ll
> > defines "SYMBOL ([[:alpha:]_][[:alnum:]_]+|[[:alpha:]][[:alnum:]_]*)".
> >
> > While xml/src/XmlAgent.cc accepts any string:
> >
> > else if (!xmlStrcmp(confAttr, (const xmlChar *)"symbol"))
> > {
> > y2debug("Symbol found");
> > YCPSymbol sym((const char *)lastChild->content);
> > return YCPSymbol(sym);
> > }
> >
> > IHMO, we should make that a little more robust. I am not sure if
> > autoyast is the only part of yast2 where xml code provided by user is
> > parsed into ycp.
> >
> > Any suggestions?
> >
> > Tschuess,
> > Thomas Fehr
>
> Well,
> I read that code quite lot in recent weeks due to ruby conversion
> project and it is not surprise that there is such inconsistencies, there
> are even more inconsistencies in ycp, its parser and implementation of
> other components that builds ycp values from own values ( like perl,
> ruby bindings or packager component )
> I think that in this case the most correct solution is let YCPSymbol
> constructor decide what is correct symbol ( downside is that it is not
> possible to change return value to nil if something is wrong, as we do
> not use factory to build values and also we do not support exceptions
> in constructor ). So how to have it? Because symbol allows any string
> even containing spaces, \r, beep sound or even more strange chars, it
> is not easy to add it to parser in plain parts after backslash. But
> solution can be inspired by ruby, that already have symbol and if
> symbol contains strange characters you can enclose symbol into
> quotes...so you can have :"tr=1". To allow such change it needs to
> modify
> 1) parser to recognize string after backslash (something like
> `<string>: rule)
> 2) modify toString method to properly construct ycp code ( BTW that
> reminds me, that due to low usage of toString method, the method
> contains bugs for some constructs like switch statement )
I do not think it makes sense to allow straneg characters in a symbol.
I would make the constructor of YCPSymbol to that it does not accept
invalid characters in a symbol. To not add unneeded checking to the
case where YCPSymbol constructor is called from Parser (where validity
of strict is encured anyway), I added a boolean defaulting to false
to YCPSymbol constructor. If it is true constructor checks string
and restricts it to valid characters. Only change in yast2-xml would
be to call constructor with true as second parameter and check
constructed Symbol content, if it is not what is expected treat the
entry as YCPString (would would happend anyway if e.g. type string
is mistyped).
Anyway this is more or less academic, since we will most probably not
have YCP in 13.x and RC2 is a little too late to add this code to
SLES11 SP3 code base.
Diff are attached...
Tschuess,
Thomas Fehr
--
Thomas Fehr, SuSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg)
Tel: +49-911-74053-0, Fax: +49-911-74053-482, Email: [email protected]
GPG public key available.
diff --git a/libycp/src/YCPSymbol.cc b/libycp/src/YCPSymbol.cc
index 3ab9dd3..e3cee14 100644
--- a/libycp/src/YCPSymbol.cc
+++ b/libycp/src/YCPSymbol.cc
@@ -22,17 +22,31 @@
#include "ycp/Bytecode.h"
#include "ycp/Xmlcode.h"
+inline string allowedString(const string& s)
+ {
+ string::const_iterator i=s.begin();
+ while( isalpha(*i) || *i=='_' )
+ ++i;
+ if( i!=s.begin() )
+ {
+ while( isalnum(*i) || *i=='_' )
+ ++i;
+ }
+ if( i!=s.begin() && s.front()=='_' && distance(s.begin(),i)==1 )
+ i=s.begin();
+ return string(s.begin(),i);
+ }
// YCPSymbolRep
-YCPSymbolRep::YCPSymbolRep(const char *s)
- : v(Ustring (*SymbolEntry::_nameHash, s))
+YCPSymbolRep::YCPSymbolRep(const char *s, bool check)
+ : v(Ustring (*SymbolEntry::_nameHash, (check?allowedString(s):s)))
{
}
-YCPSymbolRep::YCPSymbolRep(string s)
- : v(Ustring (*SymbolEntry::_nameHash, s))
+YCPSymbolRep::YCPSymbolRep(string s, bool check)
+ : v(Ustring (*SymbolEntry::_nameHash, (check?allowedString(s):s)))
{
}
@@ -101,7 +115,7 @@ fromStream (bytecodeistream & str)
}
-YCPSymbol::YCPSymbol (bytecodeistream & str)
- : YCPValue (new YCPSymbolRep (fromStream (str)))
+YCPSymbol::YCPSymbol (bytecodeistream & str, bool check)
+ : YCPValue (new YCPSymbolRep (fromStream (str),check))
{
}
diff --git a/libycp/src/include/ycp/YCPSymbol.h b/libycp/src/include/ycp/YCPSymbol.h
index e297376..6d7744b 100644
--- a/libycp/src/include/ycp/YCPSymbol.h
+++ b/libycp/src/include/ycp/YCPSymbol.h
@@ -47,12 +47,12 @@ protected:
/**
* Creates new symbol from a const char *pointer.
*/
- YCPSymbolRep(const char *s);
+ YCPSymbolRep(const char *s, bool check);
/**
* Creates a new symbol from a string.
*/
- YCPSymbolRep(string s);
+ YCPSymbolRep(string s, bool check);
public:
/**
@@ -102,9 +102,9 @@ class YCPSymbol : public YCPValue
{
DEF_COMMON(Symbol, Value);
public:
- YCPSymbol(const char *s) : YCPValue(new YCPSymbolRep(s)) {}
- YCPSymbol(string s) : YCPValue(new YCPSymbolRep(s)) {}
- YCPSymbol(bytecodeistream & str);
+ YCPSymbol(const char *s, bool check=false) : YCPValue(new YCPSymbolRep(s,check)) {}
+ YCPSymbol(string s, bool check=false) : YCPValue(new YCPSymbolRep(s,check)) {}
+ YCPSymbol(bytecodeistream & str, bool check=false);
};
#endif // YCPSymbol_h
diff --git a/src/XmlAgent.cc b/src/XmlAgent.cc
index b4b009b..620cfac 100644
--- a/src/XmlAgent.cc
+++ b/src/XmlAgent.cc
@@ -61,8 +61,12 @@ YCPValue XmlAgent::SetType(xmlNodePtr node) {
else if (!xmlStrcmp(confAttr, (const xmlChar *)"symbol"))
{
y2debug("Symbol found");
- YCPSymbol sym((const char *)lastChild->content);
- return YCPSymbol(sym);
+ YCPSymbol sym((const char *)lastChild->content,true);
+ if(strcmp(sym->symbol_cstr(),(const char*)lastChild->content)!=0)
+ y2warning("invalid symbol. xml:\"%s\" symbol:\"%s\". Using as string.",
+ lastChild->content,sym->symbol_cstr());
+ else
+ return YCPSymbol(sym);
}
else if (!xmlStrcmp(confAttr, (const xmlChar *)"integer") )
{
