[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 at 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 at 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