Re: [PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Dmitry Timoshkov]

"Saulius Krasuckas"  wrote:
> +static void safe_strcatA(char *dst, DWORD dst_len, const char *src)
> +{
> - UINT length;
> - char *buff;
> -
> - length = lstrlenA(dst);
> - length += lstrlenA(src);
> - buff = HeapAlloc(GetProcessHeap(), 0, length + 1);
> -
> - lstrcpyA(buff, dst);
> - lstrcatA(buff, src);
> - lstrcpynA(dst, buff, dst_len);
> -
> - HeapFree(GetProcessHeap(), 0, buff);
>
> +}

This kind of "safe" wrappers doesn't makes the code be safer in reality,
needlessly clobbering the code instead.

[PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Saulius Krasuckas]

* On Tue, 4 Jul 2006, Dmitry Timoshkov wrote:
> - "Saulius Krasuckas"  wrote:
> > +static void safe_strcatA(char *dst, DWORD dst_len, const char *src)
> > +{
> > - UINT length;
> > - char *buff;
> > -
> > - length = lstrlenA(dst);
> > - length += lstrlenA(src);
> > - buff = HeapAlloc(GetProcessHeap(), 0, length + 1);
> > -
> > - lstrcpyA(buff, dst);
> > - lstrcatA(buff, src);
> > - lstrcpynA(dst, buff, dst_len);
> > -
> > - HeapFree(GetProcessHeap(), 0, buff);
> >
> > +}
>
> This kind of "safe" wrappers doesn't makes the code be safer in reality,
> needlessly clobbering the code instead.

By "safe" I meant it doesn't overflow buffer.  Does it do?
But well, would your reactions was the the same if I'd called this 
local_strcatA(), uhm?

[PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Dmitry Timoshkov]

"Saulius Krasuckas"  wrote:
> By "safe" I meant it doesn't overflow buffer.  Does it do?

There are other ways to achieve that. Once Alexandre already commented
out an almost the same approach someone tried to port from ReactOS to
Wine.
> But well, would your reactions was the the same if I'd called this 
> local_strcatA(), uhm?

My reaction has nothing to do with the function name.

[PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Saulius Krasuckas]

* On Tue, 4 Jul 2006, Dmitry Timoshkov wrote:
> - "Saulius Krasuckas"  wrote:
> > By "safe" I meant it doesn't overflow buffer.  Does it do?
>
> There are other ways to achieve that. Once Alexandre already commented 
> out an almost the same approach someone tried to port from ReactOS to 
> Wine.

Hm, I don't remember this.  Can you give me an example of a nice way, 
please?  It was one "if" that I removed in my patch.  Maybe you mean this 
way?

If so, I don't understand how embracing every testable function call by an 
"if" statement looks better than local strcat-wrapper.

[PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Dmitry Timoshkov]

"Saulius Krasuckas"  wrote:
> > There are other ways to achieve that. Once Alexandre already commented 
> > out an almost the same approach someone tried to port from ReactOS to 
> > Wine.
>
> Hm, I don't remember this.  Can you give me an example of a nice way, 
> please?  It was one "if" that I removed in my patch.  Maybe you mean this 
> way?

Yes, that's a perfect way of checking for a possible buffer overflow,
there is no need to complicate things.
> If so, I don't understand how embracing every testable function call by an 
> "if" statement looks better than local strcat-wrapper.

strcat wrapper creates an illusion that it solves some problem when it
actually doesn't.

[PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Saulius Krasuckas]

* On Tue, 4 Jul 2006, Dmitry Timoshkov wrote:
> - "Saulius Krasuckas" wrote:
> > It was one "if" that I removed in my patch.  Maybe you mean this way?
>
> Yes, that's a perfect way of checking for a possible buffer overflow, 
> there is no need to complicate things.

It may be perfect from some POVs, but it creates some conditional 
branching which I was thinking we're are trying to avoid in WRT suite, 
for example branching used in checking for version-dependent stuff.
> strcat wrapper creates an illusion that it solves some problem when it 
> actually doesn't.

So you agree there is a problem which isn't solved in current test code?

[PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Dmitry Timoshkov]

"Saulius Krasuckas"  wrote:
> > strcat wrapper creates an illusion that it solves some problem when it 
> > actually doesn't.
>
> So you agree there is a problem which isn't solved in current test code?

I have no idea how it's possible to make such a conclusion from what I've said.

[PATCH 1/2] kernel/tests: Implement safe local strcat function.

[Saulius Krasuckas]

* On Wed, 5 Jul 2006, Dmitry Timoshkov wrote:
> - "Saulius Krasuckas" wrote:
> > > strcat wrapper creates an illusion that it solves some problem when 
> > > it actually doesn't.
> >
> > So you agree there is a problem which isn't solved in current test 
> > code?
>
> I have no idea how it's possible to make such a conclusion from what 
> I've said.

As this already doesn't relate to development, I am switching to private 
mode ;)