C++ dll woes (solved)

[JohnOne]

Here is the code in dll

LPWSTR __stdcall tmppath()
{
LPWSTR path[MAX_PATH];
if(GetTempPath(MAX_PATH, *path)){
  return *path;
}
else{
  return L"0";
}

Here Autoit code

$adll = DllCall("testdll.dll", "wstr", "tmppath")
If Not @error Then
MsgBox(0, "Return", VarGetType($adll[0]) & " " & $adll[0] & " Length " & StringLen($adll[0]))
Else
MsgBox(0, "Error", @error)
EndIf

The problem is that sometimes it crashes Autoit, and sometimes it returns my temp dir.

What am I doing wrong here? (besides trying to code c++)

Edited January 26, 2012 by JohnOne

[Jon]

path is getting freed after the dll closes.

[JohnOne]

I'm not sure I get that.

If I use DllOpen, and then sleep for a while after the return, it still crashes before DllClose.

[Jon]

When the function returns "path" is deallocated. Sometimes that memory is cleared (crash). Sometimes it will be left alone for a while and that's when it "seems" to work. You could allocate the memory area on DllOpen as a global and use that. Or pass it a string as a parameter and reuse that rather than trying to do it as a return type.

Edited January 20, 2012 by Jon

[JohnOne]

Thanks for the tip Jon, I'll have a pop at doing it that way.

[JohnOne]

I cant get my head around how to do that

This is the latest attempt.

extern "C" int __stdcall tmppath(LPWSTR[MAX_PATH]);
int __stdcall tmppath(LPWSTR rtn)
{
if(GetTempPath(MAX_PATH, rtn)){
  return 1;
}
else{
  return 0;
}

$hDll = DllOpen("codedll.dll")
Local $struct = "char[260]"
Local $dllstruct = DllStructCreate($struct)
If @error Then Exit
$adll = DllCall($hDll, "int", "tmppath","wstr",$dllstruct) ; tried wstr*, wstr, struct, struct*, (struct* + struct crashes autoit)
If Not @error Then
$rtn  = DllStructGetData($dllstruct,1)
MsgBox(0, "Return", VarGetType($rtn) & " " & $rtn & " Length " & StringLen($rtn))
Else
MsgBox(0, "Error", @error)
EndIf
DllClose($hDll)

Output is "String Length 0"

Edited January 20, 2012 by JohnOne

[wraithdu]

I think your declaration should be

extern "C" int __stdcall tmppath(LPWSTR);

And autoit

$adll = DllCall($hDll, "int", "tmppath", "wstr", "")
If Not @error Then
    MsgBox(0, "Return", $adll[1] & " " & $adll[1] & " Length " & StringLen($adll[1]))
Else
    MsgBox(0, "Error", @error)
EndIf

[JohnOne]

wraithdu, that was exactly it.

Thank you for the time and help gentlemen, I appreciate it.

[Richard Robertson]

I'd recommend passing both a buffer and a length as parameters, to avoid buffer overruns.

[JohnOne]

I'm not sure what that means, I thought MAX_PATH was the largest size you could have.

I wouldn't know what size the the actual string is to get it precise enough to pass in.

Of course I'm probably misunderstanding your tip.

[Richard Robertson]

Actually file names can be longer than MAX_PATH when using certain extended versions of Windows functions. But that wasn't the point of my tip.

The buffer that is allocated in AutoIt is something like char[x] where x is a number. You need to tell your function what x is so that the function doesn't write more data than that.

function(buffer, x)

And then you can't write more than x characters inside the function. If you do, you either throw a runtime error (in checked environments) or screw up potentially sensitive parts of the calling program.

[JohnOne]

I know you can pass it in that way, but I'm trying to understand the benefits of doing so over defining a size

in the C++ code.

[Valik]

Buffer overruns are an easy mistake to make, especially for a new programmer. But they are evil. On a good day a buffer overrun will just be a crash. On a bad day you'll leave a buffer overrun in an elevated process and it will be trivial to find and exploit to execute arbitrary code (elevated).

[wraithdu]

I'd recommend passing both a buffer and a length as parameters, to avoid buffer overruns.

In this implementation it's not necessary. The AutoIt wstr type is some huge size, in fact I'm not even sure it can overrun (devs?). Additionally he's already telling GetTempPath that MAX_PATH is the size of AutoIt's string buffer, which is fine. I mean sure, you could go all out and define your own buffer with DllStructCreate and pass extra params to the DLL function, and I'm not arguing that isn't the best way to do it from a coding standpoint, but is that really necessary here? I don't think so. That's the whole point of the wstr type.

Grumble. I almost didn't write this because I expected Valik's response. The brain-finger filter learns every day...

Edited January 21, 2012 by wraithdu

[Valik]

I wonder how many exploits have been created because the author of a piece of code made a conscious choice to ignore safety because "it's not really necessary here"? If for no other reason then a trivial function where it isn't necessary is the best place to learn to write functions correctly since the negative side effects are minimal if it's done wrong.

[wraithdu]

To satisfy my curiosity, is the AutoIt wstr type exploitable? Can it be overrun, or will it resize indefinitely (within the limits of available memory), or eventually just crash the script?

Edited January 21, 2012 by wraithdu

[Valik]

For input it will be 65536 characters minimum but expand beyond that if you pass in a very large string. However, it is possible the callee could write to the buffer and overrun it.

[JohnOne]

Do you think it would be wise to define a MAX_SIZE in the c++ code instead of using MAX_PATH?

after all, in this case it's only after the @TempDIR, and I suspect it would be unusual for that to

be a massive size.

[Richard Robertson]

You shouldn't use hard coded sizes when receiving buffers from other contexts.

[Beege]

I wonder how many exploits have been created because the author of a piece of code made a conscious choice to ignore safety because "it's not really necessary here"? If for no other reason then a trivial function where it isn't necessary is the best place to learn to write functions correctly since the negative side effects are minimal if it's done wrong.

I was going to bring up the original xbox game save exploits, but after some reading I found those were cause by buffer underrun, not overrun. Two different worlds apparently.