On Mon, Apr 29, Josef Reidinger wrote:

On Mon, 29 Apr 2013 16:30:20 +0200 Thomas Fehr <fehr@suse.de> 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: fehr@suse.de GPG public key available.

On Tue, Apr 30, Josef Reidinger wrote:

...

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).

Well, few comments to diff. Choose to filter out invalid characters is valid approach. Problem is that code don't filter out invalid characters if I read code properly. Code just choose first substring with valid characters. Code with `test=1 construct `test instead of `test1 . So I think it should be properly documented if it is intention and better function name should be choosen.

Indeed, it returns the match of the regular expression used in scanner.ll for symbol ("([[:alpha:]_][[:alnum:]_]+|[[:alpha:]][[:alnum:]_]*)" at start of string. So there is not really valid or invalid characters (e.g. "_22" is valid, "22_" is invalid, "_" is invalid, "__" is valid). I recognized this while implementing it, but I consider the name more or less ok, it returns the valid part of given string.

Also I think that you should inside constructor call y2warning or y2error that some characters are invalid and not let it on constructor user. And last but not least default value

Indeed a y2warning would make sense.

for constructor should be true from my point of view ( so by default filter out character ). So other code that use SymbolConstructor like various bindings get automatic valid symbol. We should be by default on safe side.

I would assume most constructed YCPSymbol values are from parsing valid ycp code and not from xml-to-ycp, therefore the default. It does not impose added overhead and keeps the previous behavior on all unchanged code. 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: fehr@suse.de GPG public key available. -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org