Crash bei TBytes Typecast

[TurboMagic]

Hallo,

ein Nutzer der DEC hat einen Crash gemeldet. Ich kann den mit dessen
Beispielprogramm nachstellen, ich verstehe aber noch nicht warum das passiert.

Es gibt diese Typdeklarationen:

Delphi-Quellcode:

  /// <summary>
  ///   Replacement for PByteArray
  /// </summary>
  PUInt8Array = ^TInt8Array;
  TInt8Array = array[0..MaxInt-1] of Byte;

Der Crash passiert hier drin:

Delphi-Quellcode:

procedure TDECCipherModes.EncodeGCM(Source, Dest: PUInt8Array; Size: Integer);
var
  PlainText,
  CipherText : TBytes;
begin
  if (Size > 0) then
  begin
    PlainText  := TBytes(@Source^);
    SetLength(PlainText, Size);
Crash>    CipherText := TBytes(@Dest^);
    SetLength(CipherText, Size);
  end
  else
  begin
    SetLength(PlainText, 0);
    SetLength(CipherText, 0);
  end;

  FGCM.EncodeGCM(PlainText, CipherText, Size);
end;

Es gibt da eine Zugriffsverletzung.
Das SetLength(PlainText, Size) und SetLength(CipherText, Size) habe ich schon ergänzt.

EncodeGCM wird von dieser Methode aus aufgerufen:

Delphi-Quellcode:

procedure TDECCipherModes.Encode(const Source; var Dest; DataSize: Integer);
begin
  EncodeGCM(@Source, @Dest, DataSize);
end;

Debugge ich doert hin liefert Assigned(@Dest) unter Watches true

Das Testprogramm übergibt diese Variablen an Encode:

ciphText, refCipherText : Array[0..3] of LongWord;

1. Warum crasht der Typecast CipherText := TBytes(@Dest^);

2. Kann man das EncodeGCM evtl. irgendwie besser schreiben?

[himitsu]

Natürlich muß das knallen

Lösung: SetLength und danach Move vom Parameter in diese Variable.


TBytes ist ein dynamisches Array, welches wie String/AnsiString
eine Kontrollstreuktur mit Referenzzählung und Längenangabe enthält,
was bei einem statischen Array fehlt.

Andersrum geht es aber, also TByte in einen Pointer auf ein statisches Array zu casten,
genauso wie String zu PChar.


Ja, PChar zu String "geht", aber das ist kein Cast, sondern CompilerMagic,
da hierfür in eine entsprechende Funktion der System.pas umgeleitet wird.

[TurboMagic]

Hallo,

danke für die Infos!
Damit kann ich es sicher vorübergehend fixen.
Eigentlich will man aber vermutlich das Umkopieren der Daten vermeiden,
damit wäre es wohl stärker umzubauen, was vorher analysiert werden muss.
Aber natürlich ist es wichtig den Crash schnellstmöglich zu beseitigen,
selbst wenn das die Performance vorübergehend verschlechtert.

[jaenicke]

Ich kenne den restlichen Quelltext nicht, aber für mein Empfinden wäre es, wenn die Zielgröße feststeht (wie hier), die sauberste Variante, wenn der Aufrufer nur korrekt dimensionierte Arrays übergeben darf.

Dann werden dahinter nur Pointer übergeben, was auch performancemäßig sauber ist. Wenn dann noch FGCM.EncodeGCM einfach PUInt8Array bekäme...

[Kas Ob.]

Zitat von TurboMagic:

Hallo,

ein Nutzer der DEC hat einen Crash gemeldet. Ich kann den mit dessen
Beispielprogramm nachstellen, ich verstehe aber noch nicht warum das passiert.

Es gibt diese Typdeklarationen:

Delphi-Quellcode:

  /// <summary>
  ///   Replacement for PByteArray
  /// </summary>
  PUInt8Array = ^TInt8Array;
  TInt8Array = array[0..MaxInt-1] of Byte;

Der Crash passiert hier drin:

Delphi-Quellcode:

procedure TDECCipherModes.EncodeGCM(Source, Dest: PUInt8Array; Size: Integer);
var
  PlainText,
  CipherText : TBytes;
begin
  if (Size > 0) then
  begin
    PlainText  := TBytes(@Source^);
    SetLength(PlainText, Size);
Crash>    CipherText := TBytes(@Dest^);
    SetLength(CipherText, Size);
  end
  else
  begin
    SetLength(PlainText, 0);
    SetLength(CipherText, 0);
  end;

  FGCM.EncodeGCM(PlainText, CipherText, Size);
end;

Es gibt da eine Zugriffsverletzung.
Das SetLength(PlainText, Size) und SetLength(CipherText, Size) habe ich schon ergänzt.

EncodeGCM wird von dieser Methode aus aufgerufen:

Delphi-Quellcode:

procedure TDECCipherModes.Encode(const Source; var Dest; DataSize: Integer);
begin
  EncodeGCM(@Source, @Dest, DataSize);
end;

Debugge ich doert hin liefert Assigned(@Dest) unter Watches true

Das Testprogramm übergibt diese Variablen an Encode:

ciphText, refCipherText : Array[0..3] of LongWord;

1. Warum crasht der Typecast CipherText := TBytes(@Dest^);

2. Kann man das EncodeGCM evtl. irgendwie besser schreiben?

And that why i hate untyped parameter !

the problem as pointed by himitsu, to catch or solve all of such bugs you need to declare them as constants so the compiler will not access them for writing , try to change this
procedure TDECCipherModes.EncodeGCM(Source, Dest: PUInt8Array; Size: Integer); into
procedure TDECCipherModes.EncodeGCM(const Source, Dest: PUInt8Array; Size: Integer);

[Kas Ob.]

Also const will solve this for every case, it is a remedy for now and will make these undefined behavior disappear for wide range cases but not all ! keep that in mind

The problem with untyped, is that they allow you to pass almost anything, in case combining strings with array it will work as the refcount is at the same place, the problem spur with when unmanaged typed passed as untyped parameter than casted away as managed type, now the compiler with its final destination will access the refcount which doesn't exist and will grab what ever there either on stack or on the heap then the fun starts.

In different example passing integer as untyped parameter to "TDECCipherModes.Encode(const Source;" then casted ( in this case it is not casted right away as it is passed as pointer "PUInt8Array" using "@Source", now "TDECCipherModes.EncodeGCM(Source, Dest: PUInt8Array;" will handle the original integer with no refcount or Count/Size as it should have, these will be right before the pointer that passed, most likely on the stack, now we are in the unknown, if the stack hold 0 or $FFFFFFFF(-1) or something else, without suggested "const" it will adjust the refcount because it is managed in its eye, compiler will generate the ref adjusting code that will crash (may be but not always and that why it passed tests for long time not being detected),

"const" will solve most of these cases, but not all !

The only fix that will be valid is to re-write "TDECCipherModes.EncodeGCM(Source, Dest: PUInt8Array;" change the type from array to simple pointer, this will solve this wide range of errors and wrong doing, internally do what every you want but don not use casting, simple walk the buffer byte by byte.

Also "TBytes(@Source^);" and "TBytes(@Dest^);" has the same undefined behavior invoking wrong casting, crashing when handling Dest i mere chance it could be with Source too.

[TurboMagic]

Hallo,

danke für alle Tipps.
Ich habe jetzt diese Methoden in Bezug auf die Datentypen der Parameter umgebaut.
Es crasht nicht mehr und die Unit Tests funktionieren alle bis auf einen. Der hatte aber
vorher schon nicht funktioniert, da wollte ich mal bei Gelegenheit ran. Das sollte aber
anders gelagert sein.

Das Testprogramm des Melders des Crashes scheint immer noch nicht ganz richtig zu laufen,
da soll der sich aber mal auf meinen Kommentar zurückmelden, der Code ist ja jetzt im
Entwicklungszweig auf Github für ihn verfügbar.

Wenn die Encode Seite so richtig sein sollte, dann werde ich auch die Decode Seite entsprechend
umbauen und danach aufräumen.

[Kas Ob.]

You mentioned a failing test, my XE8 doesn't compile the test project, so i don't know which test is failing, if you want i can have a look and try to help with that, but for that i need simpler and separated test for the failing test, one small code showing the failure.