Looks fine now, thanks for making the changes. For some reason webrev seems to think that 38 lines have changed in auto_parse_manifest.c when far fewer have actually changed .. hmm.
Alok On Fri, 6 Feb 2009, William Schumann wrote: > Alok, > Implemented all of your suggestions and reissued webrev. Comments below. > > Alok Aggarwal wrote: >> On Thu, 29 Jan 2009, William Schumann wrote: >> >> >>> Alok, >>> Responses below. >>> >>> Alok Aggarwal wrote: >>> >>>> On Tue, 27 Jan 2009, William Schumann wrote: >>>> >>>> >>>>> Passwords should be encrypted in manifests, not by the installer code. >>>>> >>>>> The basic fix was simple - remove encryption calls from the code - but >>>>> it was noted that there could be XML entities encoded in the password, >>>>> so XML entities will now be decoded. The XML decoding is basic and has >>>>> some shortcuts, such as decoding only 1-byte numeric values. It is >>>>> expected that the AI manifests will soon be restructured and then >>>>> should >>>>> be parsed with established parsers, such as in libxml2. >>>>> >>>>> Bugs fixed along the way: >>>>> - values with spaces were being truncated at the first space >>>>> - apostrophes used as value delimiters were not working >>>>> - some typos with keyword name not matching intended storage variable >>>>> name >>>>> >>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4246 >>>>> http://cr.opensolaris.org/~wmsch/bug-4246/ >>>>> >>>> auto_parse.c: lines 581-583: What happens if there >>>> are multiple blank spaces either one after the other >>>> or separated by tokens? >>>> >>> Spaces are compressed to a single space - fixed. >>> >> >> Okay. >> >> >>>> auto_parse.c: decode_xml_entities(): This code looks >>>> complicated. Given that the SC manifest is going to eventually move out >>>> to SMF enhanced profiles, I do >>>> wonder whether it's worth the effort to write this code >>>> that's going to be thrown away once we move over to SMF >>>> enhanced profiles. >>>> >>> 90 lines of code here. The effort would be in reviewing it and dealing >>> with possible bugs. >>> >> >> Maintainability is the other area that would need more >> effort than usual. >> > Given that fully functional XML parsing code will be used in the near > future, I think that supporting the use of XML elements in the SC manifest > can wait until then. Removed decode_xml_entities() XML parsing code, filing > it for possible use later. >> auto_parse.c: line 627: I don't think we want to do gratuitous truncation >> (say, if it's a password value). >> A failure is more appropriate. >> > Added failure path for bad passwords. >> auto_parse.c: line 667: shouldn't this be zero unless >> the code didn't catch an error condition else where? >> > Traced bad errno to auto_parse_manifest.c where a malloc was given a length > of -1 when optional XML element was missing. Fixed and removed nits. >> auto_parse.c: line 691: I don't think this should get >> logged to the console, logging to the install_log should >> be adequate. >> > Removed. > William >> >>> Another XML parsing problem is that it is assumed that the SC value is on >>> the same line as the "propval" keyword and AI will fail if it isn't. >>> >>> Do you know a time frame for moving to SMF enhanced profiles? >>> >> >> I don't know what the plans are in this area. >> >> >>>> Lastly, which cases did you test while testing out these >>>> changes? >>>> >>> Tried values: >>> - single, multiple spaces >>> - empty >>> - all possible (5) XML entities; XML decimal, octal encodings >>> - invalid XML entities, missing terminator >>> - XML entities adjacent, at beginning of line, at end of line >>> - value longer than VALUE_SIZE (added warning on overflow) >>> >> >> Thanks. >> >> Alok >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
