[caiman-discuss] Code review request for: 4344

[Dave Miner]

Joseph J VLcek wrote:
> /opt/onbld/bin/webrev
> Dave Miner wrote:
>> Joseph J VLcek wrote:
>>> Thanks Dave,
>>>
>>> Comments in line below.
>>>
>>> Joe
>>>
>>> Dave Miner wrote:
>>>> Joseph J VLcek wrote:
>>>>> Thanks Dave!,
>>>>>
>>>>> My comments are in line below and the updated webrev to address 
>>>>> code review comments available at:
>>>>>
>>>>> http://cr.opensolaris.org/~joev/bug4344_B
>>>>>
>>>>
>>>> ict.c
>>>>
>>>> 81,91: extraneous space
>>>
>>> OK
>>>
>>> I'll run hg nits again too.
>>>
>>>>
>>>> 103: just use calloc at 100
>>>
>>> Will do.
>>>
>>>>
>>>> My bad for not noticing this earlier, but there's no way the login 
>>>> name can contain a single quote, it's not a legal character.  The 
>>>> changes in the rest of this file are not needed (though admittedly 
>>>> harmless). Similarly, escaping the login name in 
>>>> perform_slim_install.c is unnecessary.
>>>
>>> Actually I tested it and it is possible. I manually (not with the 
>>> installer) created a login named "gu'est" on my test system and was 
>>> able to su and ssh to it. So I think it is possible to have a login 
>>> name with a quote.
>>>
>>>
>>> joevhome > ssh "gu'est"@129.148.174.230
>>> Password:
>>> Last login: Fri Oct 31 20:41:26 2008 from punchin-client-
>>> Sun Microsystems Inc.   SunOS 5.11      snv_99  November 2008
>>> -bash-3.2$ tail -5 /etc/passwd
>>> nobody4:x:65534:65534:SunOS 4.x NFS Anonymous Access User:/:
>>> guest:x:101:10:Betty Boop Guest:/export/home/guest:/bin/bash
>>> gu'est:x:101:11:Betty Boop Guest:/export/home/gu'est:/bin/bash
>>> gk:x:0:1:Gatekeeper:/opt/onbld/gk:/usr/bin/csh
>>> xvm:x:60:60:xVM User:/:
>>> -bash-3.2$ pwd
>>> /export/home/gu'est
>>> -bash-3.2$ exit
>>> logout
>>> Connection to 129.148.174.230 closed.
>>>
>>>
>>> I agree it is not likely to have a login name with a quote... but 
>>> since my investigation indicated it does seem possible I provided 
>>> support for it.
>>>
>>> How do you advise, should the installer provide support for a login 
>>> that contains quotes? I suspect doing so may very well break other 
>>> code someplace.
>>>
>>
>> Interesting.
>>
>> passwd(4) doesn't allow that in the supported set, which can be 
>> verified if you look at the valid_login() function in ON.  It'll 
>> certainly cause a lot of problems in shell scripts.  The installer 
>> absolutely should not be allowing such a string to come through, and 
>> rejecting it if attempted.
> 
> If this is the case is there any reason why ict_escape shouldn't be 
> moved out of ICT and become static function "fix_escape" in 
> perform_slim_install.c?
> 

To me, the better solution would be for libict to encapsulate this 
itself.  In other words, rather than having the high-level code in 
perform_slim_install construct a shell invocation itself to invoke the 
ICT's, have libict provide an API that does that (if that's what it 
needs to do, I think that was to some extent a stopgap solution?), and 
that way libict can handle the escaping internally, because it's really 
just an implementation detail.

Dave

> Joe
> 
>>
>> Dave
>>
>>> Joe
>>>
>>>
>>>>
>>>> Dave
>>>>
>>>>
>>>>> Dave Miner wrote:
>>>>>> Joseph J VLcek wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Can two people please do a review for the fix for bug:
>>>>>>>
>>>>>>> 4344 ICT fails to create user account if real name contains an 
>>>>>>> apostrophe
>>>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4344
>>>>>>>
>>>>>>> The webrev is available at:
>>>>>>> http://cr.opensolaris.org/~joev/bug4344/
>>>>>>>
>>>>>>
>>>>>> ict.c:
>>>>>>
>>>>>> 71: perhaps just ict_escape() or something?  current name is 
>>>>>> pretty clumsy
>>>>>
>>>>> changed to your suggestion.
>>>>>
>>>>>>
>>>>>> 74: these aren't pathnames, ergo MAXPATHLEN is the wrong 
>>>>>> constant.  In reality, for this to be generally useful, I'd advise 
>>>>>> malloc/realloc, because whatever you choose here is an arbitrary 
>>>>>> limit.
>>>>>
>>>>> Updated algorithm now uses malloc.
>>>>>
>>>>>
>>>>>>
>>>>>> 80,87,89,91,97: unnecessary casts
>>>>>
>>>>> fixed
>>>>>
>>>>>>
>>>>>> Evan already noted memory leaks here, I guess
>>>>>
>>>>> Addressed the memory leak issue in perform_slim_install.c
>>>>>
>>>>> I don't believe there is one in ict.c
>>>>>
>>>>>
>>>>>>
>>>>>> 357,465: the current implementation might just overflow the 
>>>>>> buffer, which isn't the same as being out of memory.  Also applies 
>>>>>> to the log messages in perform_slim_install.c
>>>>>
>>>>> With the new implementation the no_mem errors correctly reflect a 
>>>>> failure.
>>>>>
>>>>>
>>>>>>
>>>>>> 361,362: "user's"
>>>>>
>>>>> Fixed
>>>>>
>>>>>>
>>>>>> perform_slim_install.c:
>>>>>>
>>>>>> 2186-8: memory leakage is possible here
>>>>>>
>>>>>
>>>>> Fixed
>>>>>
>>>>>
>>>>> Again, thanks for the help!
>>>>>
>>>>> Joe
>>>>>
>>>>>
>>>>
>>>
>>
>