Hi everyone,

While evaluating a new peephole optimisation, I came across a null pointer dereference in the assembly language.  After looking at the original Pascal code, I came across this starting at line 525 of packages/chm/src/chmreader.pas:

procedure TChmReader.ReadWindows(mem:TMemoryStream);
var
   i,cnt,
   version   : integer;
   x         : TChmWindow;
begin
  if not assigned(fwindowslist) then
  fWindowsList.Clear;
  mem.Position:=0;
  ...

This code looks very suspicious to me because it calls fWindowsList.Clear only if fWindowsList is a null pointer.  This will instantly cause an access violation (Clear is not a class method).

Without the new optimisation, this is what the x86_64-win64 assembly language looks like:

    cmpq    $0,280(%rbx)
    jne    .Lj189
    movq    280(%rbx),%rcx
    movq    (%rcx),%rax
    call    *216(%rax)
.Lj189:

If JNE doesn't branch, then the value at 280(%rbx) is zero, and this is then copied into %rcx, then the value referenced by %rcx is stored in %rax, however because the value at 280(%rbx) is zero, then %rcx is also zero and (%rcx) is a null pointer dereference.

Kit

Hi Kit,

fwindowslist is created in the constructor, which may explain why this bug is dormant.
I assume this is supposed to be a defensive check, although fwindowslist is also accessed
later in this method without a safety check. Perhaps the "if not? assigned()" check can be omitted
since it isn't sufficient protection and the constructor should have automatically created the fwindowslist class.

My 2 cents...

On Wed, Feb 21, 2024 at 12:16 PM J. Gareth Moreton via fpc-devel <

I checked the code.
I removed the check, the check is pointless as you correctly pointed out.

There is more scary code in these units, a general overhaul would not go amiss.

Michael.