Re: [ros-dev] Re: [ros-diffs] [amunger] 21474: DHCP Client fixes and enhancements Set the correct hardware type flag. Fixes bug 651. Send option 12 on discovery, allows some DHCP servers to dynamically update DNS. Send option 61 for good measure. We

[WaxDragon]

On 4/6/06, Thomas Weidenmueller <w3seek(a)reactos.com> wrote:
>   There are several issues with this patch:
>
> >   +       /* What a strage dance */
> >  +       struct client_config *config = ifi->client->config; 
>  1) dhcp crashes for me on the line above. 
Strange, it works for me here.

>  
> >   +       char ComputerName
> > [MAX_COMPUTERNAME_LENGTH + 1]; 
>  2) use Unicode, please. Obsolete as described in 4) 
I really tire of hearing this.  None of the code in dhcp.exe is
unicode right now.  I've tried to hack on several pieces of ros only
to be told "USE UNICODE" by another dev.  Trying to convert entire
modules has always gone terribly for me.

>  
> >   +       DWORD ComputerNameSize = sizeof
> > ComputerName / sizeof ComputerName[0];
> >  +
> >  +       GetComputerName(ComputerName, & ComputerNameSize); 
>  3) missing error check, leading to buffer overflow if accessed the
>  string in ComputerName
>
> >   +       /* This never gets freed since it's
> > only called once */
> >  +       LPSTR lpCompName =
> >  +       HeapAlloc(GetProcessHeap(), 0, strlen(ComputerName) + 1); 
>  4) makes the ComputerName buffer on the stack obsolete. Only use the
>  dynamic buffer. strlen will likely cause a buffer overflow if the
>  computer name was longer than the length of the (obsolete) static buffer
>  on the stack, since the static buffer will never be initialized in this
>  case. 
I'll try to fix this.

>  
> >   +       if (lpCompName !=NULL) {
> >  +           GetComputerName(lpCompName, & ComputerNameSize); 
>  5) once again missing error check which may cause buffer overflows in
>  the following code
>
> >   +           /* Send our hostname, some dhcpds use
> > this to update DNS */
> >  +           config->send_options[DHO_HOST_NAME].data = strlwr(lpCompName); 
>  6) strlwr is POSIX ;) 
Not helpful, suggest something better.

>  
> >   +          
> > config->send_options[DHO_HOST_NAME].len = strlen(ComputerName); 
>  7) operating on the wrong buffer, may cause buffer overflow due to 5)
>
> >   +       warn("util.c read_client_conf poorly
> > implemented!"); 
>  Indeed :(
>
>  Apart from the bugs mentioned in this code, GetComputerNameExA has a few
>  bugs: 1) incorrectly returning ERROR_OUTOFMEMORY instead of FALSE and 2)
>  not checking the return value of RtlUnicodeStringToAnsiString.
>
>  - Thomas
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>  
WD
--
<Bizzeh> it even has a panda that pops out of your case and mauls
people who ask stupid questions

[Thomas Weidenmueller]

WaxDragon wrote:
>   I really tire of hearing this.  None of the code in
> dhcp.exe is
>  unicode right now.  I've tried to hack on several pieces of ros only
>  to be told "USE UNICODE" by another dev.  Trying to convert entire
>  modules has always gone terribly for me. 
I'm sorry. I didn't look at the other code. I only looked at the diff.
Anyway, I'm not blaming you, we got tons of terrible code, so I tend to
point them out when it makes sense. Especially when something crashes
for me ;)

I don't mean to offend anybody with my criticism, I just want to point
out what could and should be fixed or done differently. Sooner or later
this project needs to focus on security, so all these things have to be
fixed some day. It might not be soon, but I think we should try to keep
the number of obvious bugs in code we add as small as possible. I
realize that probably 90% of dhcp is very poor quality, but once again,
I only looked at the patch.

- Thomas

[Aleksey Bragin]

1st step is prevention in current / future commits, and 2nd step is fixing 
already developed code --- this is always better than not looking at the 
commited code and trying to fix all at once.

So this is great, and I don't think it could be thought as offensive since 
it's actually a ready how-to-improve-this-code guide :-).


WBR,
Aleksey Bragin.

----- Original Message ----- 
From: "Thomas Weidenmueller" <w3seek(a)reactos.com>
To: "ReactOS Development List" <ros-dev(a)reactos.org>
Sent: Thursday, April 06, 2006 3:50 PM
Subject: Re: [ros-dev] Re: [ros-diffs] [amunger] 21474: DHCP Client fixesand 
enhancements Set the correct hardware type flag. Fixes bug651. Send option 
12 on discovery,allows some DHCP servers to dynamically update DNS.Send 
option 61 for good measure. We


>   WaxDragon wrote:
> >   I really tire of hearing this.  None of the code
> > in dhcp.exe is
> >  unicode right now.  I've tried to hack on several pieces of ros only
> >  to be told "USE UNICODE" by another dev.  Trying to convert entire
> >  modules has always gone terribly for me. 
>  I'm sorry. I didn't look at the other code. I only looked at the diff.
>  Anyway, I'm not blaming you, we got tons of terrible code, so I tend to
>  point them out when it makes sense. Especially when something crashes
>  for me ;)
>
>  I don't mean to offend anybody with my criticism, I just want to point
>  out what could and should be fixed or done differently. Sooner or later
>  this project needs to focus on security, so all these things have to be
>  fixed some day. It might not be soon, but I think we should try to keep
>  the number of obvious bugs in code we add as small as possible. I
>  realize that probably 90% of dhcp is very poor quality, but once again,
>  I only looked at the patch.
>
>  - Thomas