On Tue, 30 Apr 2013 13:10:52 +0200 Thomas Fehr <fehr@suse.de> wrote:
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).
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. 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 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.
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.
I agree and that is nice example that if language is used by more projects, that there is bigger change that some already face such issue and it is already fixed. But maybe there will be SP4 where it can be used :)...and second thing, that even YCP doen't survive, YCPValues will survive as it is used in component system to pass data ( like DBus types in DBus ). So types will survive if we do not change component system in future to something else like DBus ( it is nice to see that YaST have something like DBus before it, but due to our fail with our popularization it is not used).
Diff are attached...
Tschuess, Thomas Fehr
Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org