Thomas Weidenmueller wrote:
> 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.
Your criticism is hugely appreciated. That's the whole point of ros-diffs :)
WaxDragon wrote:
> I'll try to fix this.
I'll give you a hand with it tonight if you need one.
Ged.
************************************************************************
The information contained in this message or any of its
attachments is confidential and is intended for the exclusive
use of the addressee. The information may also be legally
privileged. The views expressed may not be company policy,
but the personal views of the originator. If you are not the
addressee, any disclosure, reproduction, distribution or other
dissemination or use of this communication is strictly prohibited.
If you have received this message in error, please contact
postmaster(a)exideuk.co.uk
<mailto:postmaster@exideuk.co.uk> and then delete this message.
Exide Technologies is an industrial and transportation battery
producer and recycler with operations in 89 countries.
Further information can be found at www.exide.com
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.
> + char ComputerName [MAX_COMPUTERNAME_LENGTH + 1];
2) use Unicode, please. Obsolete as described in 4)
> + 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.
> + 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 ;)
> + 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
> From: "Murphy, Ged (Bolton)" <MurphyG(a)cmpbatteries.co.uk>
>
> I'm going to have a look at this tonight, but I've had a look
> before and IIRC, I had trouble hunting the bug down.
> It would be good if someone with a bit more experience in
> this area could have a look.
>
> I think Ge was working on this before Christmas,
I was
> so I'm
> hopefully gonna catch up with him tonight via IM, as I'm not
> sure if he's subscribed to ros-dev anymore.
I am, although I don't read every message anymore.
The status of the applet when I worked on it was that the UI was complete
and I was starting to work on transfering the data from the dialog to the
TCP/IP driver (via the DHCP client). That part simply isn't implemented yet.
I don't have any uncommitted code floating around.
Ge
> + DWORD HELMemoryAvilable;
> + This->HELMemoryAvilable = HEL_GRAPHIC_MEMORY_MAX;
> + *free = This->HELMemoryAvilable;
There is a small typo throughout this code
"Available"
:)
************************************************************************
The information contained in this message or any of its
attachments is confidential and is intended for the exclusive
use of the addressee. The information may also be legally
privileged. The views expressed may not be company policy,
but the personal views of the originator. If you are not the
addressee, any disclosure, reproduction, distribution or other
dissemination or use of this communication is strictly prohibited.
If you have received this message in error, please contact
postmaster(a)exideuk.co.uk
<mailto:postmaster@exideuk.co.uk> and then delete this message.
Exide Technologies is an industrial and transportation battery
producer and recycler with operations in 89 countries.
Further information can be found at www.exide.com
aleksey(a)studiocerebral.com wrote:
> Author: tretiakov
> Date: Fri Mar 31 21:47:52 2006
> New Revision: 21429
>
> URL: http://svn.reactos.ru/svn/reactos?rev=21429&view=rev
> Log:
> [AUDIT] msgina is clean.
>
>
What is the reason for these being clean signed off as clean?
Did you speak to the authors? Did you follow the conditions in the wiki?