WM_COPYDATA with and without quotes yields different results

Question

Using WM_COPYDATA to pass command line params to another app instance with Delphi XE as follows:

function DAppInstance.SendParamsToPrevInstance(AWindowHandle: THandle): Boolean;
var
  copyData: TCopyDataStruct;
  cmdParams : string;
  i : integer;
begin
  cmdParams := '';
  for i := 1 to ParamCount do
    cmdParams := cmdParams + ParamStr(i); //#1
  //cmdParams := cmdParams + '"' + ParamStr(i) + '" '; //#2
  //cmdParams := cmdParams + format('"%s" ', [ParamStr(i)]); //#3
  //cmdParams := cmdParams + format('%s;', [ParamStr(i)]); //#4

  copyData.lpData := pchar(cmdParams);
  copyData.cbData := 1 + (bytelength(cmdParams));
  copyData.dwData := WaterMark;  //ID for APP

  result := SendMessage(AWindowHandle, 
    WM_COPYDATA, 
    Application.Handle, 
    LPARAM(@copyData)) = 1;
end;

yields different results if the strings are quoted / appended to.

if #1 is used - the string comes in clean but is not usable if not quoted as filenames can have spaces and this:

C:\Users\MX4399\Research\delphi\instance\doc with spaces.doc

will be see as 3 paramaters in the end, while using #2 to quote the strings, or appending anything (#3, #4) causes

"C:\Users\MX4399\Research\delphi\instance\doc with spaces.doc"'#$FF00'궳獧

score 5 · Answer 1 · answered Sep 24 '11 at 19:42

I believe that @TOndrej has spotted the main cause of the problem. However, I think you have a second more subtle bug.

Your app which receives the WM_COPYDATA message is, I think, treating lpData as a null-terminated string. If the data is malformed then you will have a buffer overrun. I believe that is exactly what is happening in your examples but it just turns out to be benign. The marshalling of WM_COPYDATA copies just the size of buffer specified in cbData. You must make sure you don't read beyond it. A malicious app could send you a WM_COPYDATA message with data to make you do just that. Instead I recommend you use cbData when reading.

So to send the string you write:

copyData.lpData := PChar(cmdParams);
copyData.cbData := ByteLength(cmdParams))
copyData.dwData := WaterMark;

And then when you receive it you allocate a buffer and copy to that buffer based on the value of cbData.

SetString(cmdParams, PChar(copyData.lpData), copyData.cbData div SizeOf(Char));

you are right, that will be safer, I will investigate. Odd though that modifying paramstr(n) causes this not not if appended to. — MX4399, Sep 24 '11 at 20:25
You were just lucky before. The code was broken even without appending. — David Heffernan, Sep 24 '11 at 20:30

score 4 · Accepted Answer · answered Sep 24 '11 at 18:20

4

I think you meant copyData.cbdata := 1 * SizeOf(Char) + ... instead of just 1 + ....

answered Sep 24 '11 at 18:20

Ondrej Kelle

36,941
2
65
128

+1 Yes that would explain it. Of course it would be easier to write `SizeOf(Char)*(1+Length(cmdParams))` than use a special bytelength function. – David Heffernan Sep 24 '11 at 18:23
1

Actually, I think it would be safer to use the byte count in the receiving code and not bother marshalling the zero terminator. Relying on an WM_COPYDATA being zero-terminated is a recipe for buffer overruns. – David Heffernan Sep 24 '11 at 18:31
`ByteLength()` does the `SizeOf(Char)*Lenght(String)` calculation for you actually, it uses `StringElementSize()` instead of `SizeOf()`). It returns the number of bytes, not characters, used by a string. `WM_COPYDATA` expects a byte count. – Remy Lebeau Sep 24 '11 at 19:10
@Remy Yes, but it misses the null terminator and we assume that the recipient code is treating lpData as a C string. – David Heffernan Sep 24 '11 at 19:33
Changed it to `copyData.cbdata := byteLength(#0) + (bytelength(cmdParams));` and it works fine - rather logical – MX4399 Sep 25 '11 at 05:20
Taking the byte length of the null teminator literal doesn't guarantee the correct byte count to match the string encoding. You should use `StringElementSize(cmdParams)` instead of `ByteLength(#0)`. – Remy Lebeau Sep 25 '11 at 16:02

score 4 · Answer 3 · answered Sep 25 '11 at 16:20

On a separate but related note, rather then using ParamStr() (which itself has a number of known bugs) to parse the original command-line and rebuild a new string from it, you could just use GetCommandLine() to get the original command-line instead and send it as-is.

WM_COPYDATA with and without quotes yields different results

3 Answers3