Help debugging code please

[david_navigator 12]

Not sure if something has changed in 10.4.2 but the following code works under 10.3 but not 10.4.2.
The problem is the line

 move(temp[1],Buffer^, length(temp) * sizeof(WideChar));

which raises an exception if LoginMessage is empty.
Stepping through, temp = #0 which I'm guessing is why move(temp[1].... fails, but why would it have worked previously (different compiler settings ) ? 

procedure TnxdsrPluginCmdHandler.nmLoginMessage(var aMsg: TnxDataMessage);
var
  error: TnxResult;
  Buffer: PWideChar;
  BufLen: SmallInt;
  temp: WideString;
  reply: TnxnmServerlogonRpy;
begin
    BufLen := high(Reply.Buffer) + 1;
     Buffer :=AllocMem(BufLen);
    try
      if assigned(PluginEngine) then
        temp := WideString(TnxdsrServerPlugin(PluginEngine).LoginMessage)
      else
        temp := '';      
      move(temp[1],Buffer^, length(temp) * sizeof(WideChar));
      error := 0;
      BufLen := Length(Buffer);
      reply.buflen := buflen;
      copymemory(@reply.buffer, buffer, Buflen * sizeof(WideChar));
      assert(buflen = length(buffer), 'Data Checking data Lengths differ');
    except
      reply.BufLen := 0;
      error := DBIERR_NOSERVERSW;
    end;

 

Cheers

David

Edited March 17, 2021 by david_navigator

[Guest]

as you should know, a "string" is as a "array"... then, if your (string = '') then, you array is count=0... where  temp[1] will be a index not valid!!!

then, it's good pratice verify this case:

if not(Temp='') then
begin
 ... do it
end;

 

hug

Edited March 18, 2021 by Guest

[david_navigator 12]

7 hours ago, emailx45 said:

as you should know, a "string" is as a "array"... then, if your (string = '') then, you array is count=0... where  temp[1] will be a index not valid!!!

then, it's good pratice verify this case:

if not(Temp='') then
begin
 ... do it
end;

 

hug

 

Absolutely. I can see why the code fails and how to fix it, what I don't understand is why it's been working for 10 years. This routine is called 100's of times a day and Temp is virtually always an empty string, but the exception only started when I compiled the app with D 10.4.2, so I suppose my question is, why ?
Maybe there's a compiler setting which has a different default in 10.4.2 ?

[David Heffernan 2345]

1. Why are you using WideString? That's a type for COM interop. Shouldn't temp be string?
2. You leak Buffer.
3. Why do you allocate buffer so that you can copy temp to Buffer and then to reply.buffer? Why not copy direct?
4. Why use Move and CopyMemory? They do the same job. Pick one and use it consistently.
5. What do you do to defend against buffer overrun of reply.buffer?
6. If your code raises an exception, that exception is raised at a specific line, and has a specific message. That information is useful. That you didn't include it in the post suggests that you have not studied it in detail. I bet that it tells you information that would help you understand. And certainly would help us understand. What is that information?
7. The blanket exception handler also looks pretty nasty. That's going to convert all exceptions, no matter how they arise, in to the same error condition.

 

We can guess that temp[1] trips up range checking. But the exception message would remove any need for guessing. Btw, you fix that by using Pointer(temp)^ in place of temp[1].

Edited March 18, 2021 by David Heffernan

[Fr0sT.Brutal 900]

3 hours ago, david_navigator said:

I can see why the code fails and how to fix it, what I don't understand is why it's been working for 10 years. This routine is called 100's of times a day and Temp is virtually always an empty string, but the exception only started when I compiled the app with D 10.4.2, so I suppose my question is, why ?

Talking frankly, this code is piece of garbage, the fact it worked before is just a big luck. You address 1-th char of an empty string and expect it to pass.

[david_navigator 12]

24 minutes ago, David Heffernan said:

1. Why are you using WideString? That's a type for COM interop. Shouldn't temp be string?
2. You leak Buffer.
3. Why do you allocate buffer so that you can copy temp to Buffer and then to reply.buffer? Why not copy direct?
4. Why use Move and CopyMemory? They do the same job. Pick one and use it consistently.
5. What do you do to defend against buffer overrun of reply.buffer?
6. If your code raises an exception, that exception is raised at a specific line, and has a specific message. That information is useful. That you didn't include it in the post suggests that you have not studied it in detail. I bet that it tells you information that would help you understand. And certainly would help us understand. What is that information?

 

We can guess that temp[1] trips up range checking. But the exception message would remove any need for guessing. Btw, you fix that by using Pointer(temp)^ in place of temp[1].

 

Actually it's code buried deep in a third party component and I only looked at it because it started to error with D10.4.2
As I said, it worked (well at least functioned) correctly for the past 10 years under various versions of Delphi (up to 10.3)

>Why use Move and CopyMemory? They do the same job. Pick one and use it consistently.

I thought one dealt with overlapped memory & one didn't ? Though no idea why the original code used both.

I guess the 2 min fix to test that Test is not an empty string is going to turn in to a major rewrite. 

[David Heffernan 2345]

The code is something of a disaster zone. That leak looks dire. If the component contains this, then who knows what else is there!

 

As for the two minute fix, I gave that above. Replace temp[1] with Pointer(temp)^. But we're still making assumptions because we can't see the exception details.

Edited March 18, 2021 by David Heffernan

[Anders Melander 1782]

Accessing a string one char beyond the length of the string is fine because Delphi string contains an implicit zero terminating zero.

Doing the same on an empty string once worked the same but now causes an AV because the string is nil. I don't know when this changed but it was before 10.3

var
  Foo: string;
begin
  Foo := 'Hello World';
  ShowMessage(IntToStr(Ord( Foo[Length(Foo)+1] ))); // No problem

  Foo := '';
  ShowMessage(IntToStr(Ord( Foo[Length(Foo)+1] ))); // Access Violation
end;

My guess is the code predates Delphi 2009 since it uses WideString to support unicode.