View Issue Details

IDProjectCategoryView StatusLast Update
0035187FPCCompilerpublic2019-03-11 00:16
Reporter440bxAssigned ToFlorian 
PriorityhighSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Platform32 and 64 bit AMD64OSWindows and possibly othersOS Versionall
Product Version3.0.4Product Build 
Target VersionFixed in Version3.3.1 
Summary0035187: FPC generates a word access when it should generate a byte access, occasionally causing an access violation
DescriptionFor some expressions the compiler generates code that accesses a word instead of accessing a byte. When this occurs on the last accessible byte of a memory block, this causes an access violation. In the following code snippet:


procedure DumpHex(BaseAddress : pointer; BlockSize : DWORD);
const
  HexDigits : packed array[0..$F] of char = '0123456789ABCDEF';

var
  Buf : packed array[0..127] of char;

  HexPtr : ^char;

  p : pbyte;

  i : dword;
begin
  ZeroMemory(@Buf, sizeof(Buf));

  HexPtr := @Buf;

  if BlockSize > high(Buf) then BlockSize := high(Buf); // only dump 1 buffer's
                                                         // worth
  i := 0;
  while I < BlockSize do
  begin
    pchar(p) := pchar(BaseAddress) + I;

    HexPtr^ := HexDigits[p^ shr 4]; // first nibble
    inc(HexPtr);

    // this instruction causes an access violation on the last byte because
    // it tries to access a word instead of a byte.

    HexPtr^ := HexDigits[p^ and $F]; // second nibble
    inc(HexPtr);

    writeln('I : ', i,
            ' address : ', IntToHex(ptruint(p), 0), ' dump : ', Buf);

    inc(I);
  end;

  writeln(Buf); // won't get here
end;

the expression:

    HexPtr^ := HexDigits[p^ and $F]; // second nibble

generates a word access instead of a byte access.

The attached program reliably reproduces the problem.
Steps To Reproduceattempt to access the last accessible byte of a memory block.

Compile and run the attached program. Look at the code generated for the expression:

    HexPtr^ := HexDigits[p^ and $F]; // second nibble

to see the cause of the access violation.
Tagscompiler, optimization
Fixed in Revision41667
FPCOldBugId
FPCTarget
Attached Files
  • WordInsteadOfByteAccess.7z (2,351 bytes)
  • WordInsteadOfByteAccess.zip (2,985 bytes)
  • WordInsteadOfByteAccess.s_i386-linux.zip (8,866 bytes)
  • fpc-debug-output-and-tree-dump_i386-linux.zip (32,589 bytes)
  • WordInsteadOfByteAccess.s_i386-win32.zip (7,521 bytes)
  • WordInsteadOfByteAccess.s_x86_64-linux.zip (9,074 bytes)
  • WordInsteadOfByteAccess.s_x86_64-win64.zip (7,738 bytes)
  • WordInsteadOfByteAccess2.zip (2,720 bytes)
  • aoptx86.pas-35187.patch (3,040 bytes)
    Index: compiler/x86/aoptx86.pas
    ===================================================================
    --- compiler/x86/aoptx86.pas	(revision 41623)
    +++ compiler/x86/aoptx86.pas	(working copy)
    @@ -3484,24 +3484,24 @@
                           MatchOpType(taicpu(hp1),top_const,top_reg) and
                           (taicpu(hp1).oper[1]^.reg = taicpu(p).oper[1]^.reg) then
                           begin
    -                        taicpu(p).opcode := A_MOV;
    +                        //taicpu(p).opcode := A_MOV;
                             case taicpu(p).opsize Of
                               S_BL:
                                 begin
                                   DebugMsg(SPeepholeOptimization + 'var13',p);
    -                              taicpu(p).changeopsize(S_L);
    +                              taicpu(hp1).changeopsize(S_L);
                                   taicpu(hp1).loadConst(0,taicpu(hp1).oper[0]^.val and $ff);
                                 end;
                               S_WL:
                                 begin
                                   DebugMsg(SPeepholeOptimization + 'var14',p);
    -                              taicpu(p).changeopsize(S_L);
    +                              taicpu(hp1).changeopsize(S_L);
                                   taicpu(hp1).loadConst(0,taicpu(hp1).oper[0]^.val and $ffff);
                                 end;
                               S_BW:
                                 begin
                                   DebugMsg(SPeepholeOptimization + 'var15',p);
    -                              taicpu(p).changeopsize(S_W);
    +                              taicpu(hp1).changeopsize(S_W);
                                   taicpu(hp1).loadConst(0,taicpu(hp1).oper[0]^.val and $ff);
                                 end;
     {$ifdef x86_64}
    @@ -3508,7 +3508,7 @@
                               S_BQ:
                                 begin
                                   DebugMsg(SPeepholeOptimization + 'var16',p);
    -                              taicpu(p).changeopsize(S_Q);
    +                              taicpu(hp1).changeopsize(S_Q);
                                   taicpu(hp1).loadConst(
                                     0, taicpu(hp1).oper[0]^.val and $ff);
                                 end;
    @@ -3515,13 +3515,13 @@
                               S_WQ:
                                 begin
                                   DebugMsg(SPeepholeOptimization + 'var17',p);
    -                              taicpu(p).changeopsize(S_Q);
    +                              taicpu(hp1).changeopsize(S_Q);
                                   taicpu(hp1).loadConst(0, taicpu(hp1).oper[0]^.val and $ffff);
                                 end;
                               S_LQ:
                                 begin
                                   DebugMsg(SPeepholeOptimization + 'var18',p);
    -                              taicpu(p).changeopsize(S_Q);
    +                              taicpu(hp1).changeopsize(S_Q);
                                   taicpu(hp1).loadConst(
                                     0, taicpu(hp1).oper[0]^.val and $ffffffff);
                                 end;
    
    aoptx86.pas-35187.patch (3,040 bytes)
  • tw35187.patch (3,546 bytes)
    Index: tests/webtbs/tw35187.pp
    ===================================================================
    --- tests/webtbs/tw35187.pp	(nonexistent)
    +++ tests/webtbs/tw35187.pp	(working copy)
    @@ -0,0 +1,112 @@
    +program tw35187;
    +
    +{ %cpu=i386,x86_64 }
    +{ %opt=-O1 }
    +
    +{ NOTE: SIGSEGV won't trigger if GetMem is used because it allocates pages from a large pre-reserved heap. [Kit] }
    +
    +{$IFDEF TARGET_VALID}
    +{$UNDEF TARGET_VALID}
    +{$ENDIF TARGET_VALID}
    +
    +{$IFDEF WINDOWS}
    +uses
    +  Windows;
    +{$DEFINE TARGET_VALID}
    +{$ENDIF}
    +{$IFDEF UNIX}
    +uses
    +  BaseUnix, SysCall;
    +
    +function fpmprotect(Addr: Pointer; Len: PtrUInt; Prot: LongInt): LongInt; inline;
    +begin
    +  fpmprotect := Do_SysCall(syscall_nr_mprotect, TSysParam(Addr), Len, Prot);
    +end;
    +{$DEFINE TARGET_VALID}
    +{$ENDIF}
    +
    +{$IFNDEF TARGET_VALID}
    +{$ERROR No memory allocation routine available }
    +{$ENDIF TARGET_VALID}
    +
    +const
    +  TestBlock: packed array[0..127] of Char = 'The quick brown fox jumps over the lazy dog, Victor jagt zw'#148'lf Boxk'#132'mpfer quer '#129'ber den gro'#225'en Sylter Deich.'#10#13'0123456789?!"#%'#251#253#252;
    +
    +  Expected: packed array[0..255] of Char = '54686520717569636B2062726F776E20666F78206A756D7073206F76657220746865206C617A7920646F672C20566963746F72206A616774207A77946C6620426F786B846D70666572207175657220816265722064656E2067726FE1656E2053796C7465722044656963682E0A0D303132333435363738393F21222325FBFDFC';
    +
    +  HexDigits: packed array[0..$F] of Char = '0123456789ABCDEF';
    +
    +var
    +  Buf: packed array[0..255] of Char;
    +  HexPtr: PChar;
    +  P: PByte;
    +  I: DWord;
    +  HeapBlock, HeapMarker: PByte;
    +begin
    +  WriteLn(TestBlock);
    +  FillChar(Buf, SizeOf(Buf), 0);
    +
    +  { Reserve two 4K memory pages: one that is read-write followed by one that
    +    has no access rights at all and will trigger SIGSEGV if encroached }
    +{$IFDEF WINDOWS}
    +  HeapBlock := VirtualAlloc(
    +                 VirtualAlloc(nil, 8192, MEM_RESERVE, PAGE_READWRITE),
    +                 4096,
    +                 MEM_COMMIT,
    +                 PAGE_READWRITE
    +               );
    +  if not Assigned(HeapBlock) then
    +    begin
    +      WriteLn('Memory allocation failure');
    +      Halt(1);
    +    end;
    +  HeapMarker := HeapBlock + 3968; { 4096 - 128 }
    +{$ENDIF WINDOWS}
    +{$IFDEF UNIX}
    +  HeapBlock := fpmmap(nil, 8192, PROT_NONE, MAP_ANON or MAP_PRIVATE, -1, 0);
    +  if not Assigned(HeapBlock) or (fpmprotect(HeapBlock, 4096, PROT_READ or PROT_WRITE) <> 0) then
    +    begin
    +      WriteLn('Memory allocation failure');
    +      Halt(1);
    +    end;
    +
    +  HeapMarker := HeapBlock + 3968; { 4096 - 128 }
    +{$ENDIF UNIX}
    +
    +  Move(TestBlock, HeapMarker^, SizeOf(TestBlock));
    +  HexPtr := @Buf;
    +
    +  for I := 0 to SizeOf(TestBlock) - 1 do
    +  begin
    +    P := HeapMarker + I;
    +
    +    HexPtr^ := HexDigits[P^ shr 4]; { first nybble }
    +    Write(HexPtr^);
    +    Inc(HexPtr);
    +
    +    { #35187: This instruction causes an access violation on the last byte
    +        because it tries to read a word instead of a byte. }
    +
    +    HexPtr^ := HexDigits[P^ and $F]; { second nybble }
    +    Write(HexPtr^);
    +    Inc(HexPtr);
    +  end;
    +
    +{$IFDEF WINDOWS}
    +  VirtualFree(HeapBlock, 0, MEM_RELEASE);
    +{$ENDIF WINDOWS}
    +{$IFDEF UNIX}
    +  fpmunmap(HeapBlock, 8192);
    +{$ENDIF UNIX}
    +
    +  WriteLn();
    +
    +  for I := 0 to SizeOf(TestBlock) - 1 do
    +    if Buf[I] <> Expected[I] then
    +    begin
    +      WriteLn('Error at index ', I, '; expected ', Expected[I], ' got ', Buf[I]);
    +      Halt(1);
    +    end;
    +
    +  WriteLn('ok');
    +end.
    
    tw35187.patch (3,546 bytes)

Activities

440bx

2019-03-05 01:26

reporter  

WordInsteadOfByteAccess.7z (2,351 bytes)

Cyrax

2019-03-05 10:57

reporter  

WordInsteadOfByteAccess.zip (2,985 bytes)

Cyrax

2019-03-05 10:58

reporter   ~0114653

Last edited: 2019-03-05 10:58

View 2 revisions

Attaching test project (WordInsteadOfByteAccess.zip) which is made suitable to build and run in both Windows and Linux environments.

Cyrax

2019-03-05 11:03

reporter   ~0114654

FPC 3.3.1 r41584 i386-linux
Lazarus 2.1.0 r60584 i386-linux-gtk2


---

16 KB reserved at address : F7F98000
4KB have been commited at : F7F98000

I : 0 address : F7F98FF6 dump : DA
I : 1 address : F7F98FF7 dump : DADA
I : 2 address : F7F98FF8 dump : DADADA
I : 3 address : F7F98FF9 dump : DADADADA
I : 4 address : F7F98FFA dump : DADADADADA
I : 5 address : F7F98FFB dump : DADADADADADA
I : 6 address : F7F98FFC dump : DADADADADADADA
I : 7 address : F7F98FFD dump : DADADADADADADADA
I : 8 address : F7F98FFE dump : DADADADADADADADADA
An unhandled exception occurred at $08049172:
EAccessViolation: Access violation
  $08049172  DUMPHEX,  line 54 of WordInsteadOfByteAccess.lpr
  $080494CC  main,  line 127 of WordInsteadOfByteAccess.lpr

Cyrax

2019-03-05 11:05

reporter  

WordInsteadOfByteAccess.s_i386-linux.zip (8,866 bytes)

Cyrax

2019-03-05 11:06

reporter   ~0114655

Attached assembler source file of the project. (WordInsteadOfByteAccess.s_i386-linux.zip)

Cyrax

2019-03-05 11:09

reporter  

fpc-debug-output-and-tree-dump_i386-linux.zip (32,589 bytes)

Cyrax

2019-03-05 11:15

reporter   ~0114656

Cross complied the project from i386-linux to i386-win32. Here is output from that binary.

---

16 KB reserved at address : 1D0000
4KB have been commited at : 1D0000

I : 0 address : 1D0FF6 dump : DA
I : 1 address : 1D0FF7 dump : DADA
I : 2 address : 1D0FF8 dump : DADADA
I : 3 address : 1D0FF9 dump : DADADADA
I : 4 address : 1D0FFA dump : DADADADADA
I : 5 address : 1D0FFB dump : DADADADADADA
I : 6 address : 1D0FFC dump : DADADADADADADA
I : 7 address : 1D0FFD dump : DADADADADADADADA
I : 8 address : 1D0FFE dump : DADADADADADADADADA
An unhandled exception occurred at $00401780:
EAccessViolation: Access violation
  $00401780  DUMPHEX,  line 54 of WordInsteadOfByteAccess.lpr
  $00401A83  main,  line 127 of WordInsteadOfByteAccess.lpr

Cyrax

2019-03-05 11:17

reporter  

WordInsteadOfByteAccess.s_i386-win32.zip (7,521 bytes)

Cyrax

2019-03-05 11:28

reporter  

WordInsteadOfByteAccess.s_x86_64-linux.zip (9,074 bytes)

Cyrax

2019-03-05 11:31

reporter  

WordInsteadOfByteAccess.s_x86_64-win64.zip (7,738 bytes)

Cyrax

2019-03-05 11:38

reporter  

WordInsteadOfByteAccess2.zip (2,720 bytes)

Cyrax

2019-03-05 11:38

reporter   ~0114657

Attached updated test project. (WordInsteadOfByteAccess2.zip)

Cyrax

2019-03-05 14:05

reporter   ~0114661

There is a workaround for this bug in this post https://forum.lazarus.freepascal.org/index.php/topic,44549.msg313282.html#msg313282

Jeppe Johansen

2019-03-05 19:27

developer   ~0114668

It's caused by the 'var15' peephole optimization

Do-wan Kim

2019-03-07 05:22

reporter  

aoptx86.pas-35187.patch (3,040 bytes)
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 41623)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -3484,24 +3484,24 @@
                       MatchOpType(taicpu(hp1),top_const,top_reg) and
                       (taicpu(hp1).oper[1]^.reg = taicpu(p).oper[1]^.reg) then
                       begin
-                        taicpu(p).opcode := A_MOV;
+                        //taicpu(p).opcode := A_MOV;
                         case taicpu(p).opsize Of
                           S_BL:
                             begin
                               DebugMsg(SPeepholeOptimization + 'var13',p);
-                              taicpu(p).changeopsize(S_L);
+                              taicpu(hp1).changeopsize(S_L);
                               taicpu(hp1).loadConst(0,taicpu(hp1).oper[0]^.val and $ff);
                             end;
                           S_WL:
                             begin
                               DebugMsg(SPeepholeOptimization + 'var14',p);
-                              taicpu(p).changeopsize(S_L);
+                              taicpu(hp1).changeopsize(S_L);
                               taicpu(hp1).loadConst(0,taicpu(hp1).oper[0]^.val and $ffff);
                             end;
                           S_BW:
                             begin
                               DebugMsg(SPeepholeOptimization + 'var15',p);
-                              taicpu(p).changeopsize(S_W);
+                              taicpu(hp1).changeopsize(S_W);
                               taicpu(hp1).loadConst(0,taicpu(hp1).oper[0]^.val and $ff);
                             end;
 {$ifdef x86_64}
@@ -3508,7 +3508,7 @@
                           S_BQ:
                             begin
                               DebugMsg(SPeepholeOptimization + 'var16',p);
-                              taicpu(p).changeopsize(S_Q);
+                              taicpu(hp1).changeopsize(S_Q);
                               taicpu(hp1).loadConst(
                                 0, taicpu(hp1).oper[0]^.val and $ff);
                             end;
@@ -3515,13 +3515,13 @@
                           S_WQ:
                             begin
                               DebugMsg(SPeepholeOptimization + 'var17',p);
-                              taicpu(p).changeopsize(S_Q);
+                              taicpu(hp1).changeopsize(S_Q);
                               taicpu(hp1).loadConst(0, taicpu(hp1).oper[0]^.val and $ffff);
                             end;
                           S_LQ:
                             begin
                               DebugMsg(SPeepholeOptimization + 'var18',p);
-                              taicpu(p).changeopsize(S_Q);
+                              taicpu(hp1).changeopsize(S_Q);
                               taicpu(hp1).loadConst(
                                 0, taicpu(hp1).oper[0]^.val and $ffffffff);
                             end;
aoptx86.pas-35187.patch (3,040 bytes)

Do-wan Kim

2019-03-07 05:25

reporter   ~0114692

Last edited: 2019-03-07 07:30

View 3 revisions

Patch for avoiding problem.

It reduce one assembler line.

No optimization
--------------------------------------------------------------------
WordInsteadOfByteAccess.lpr:40 HexPtr^ := HexDigits[p^ and $F]; // second nibble
00401774 8b8570ffffff mov -0x90(%ebp),%eax
0040177A 660fb600 movzbw (%eax),%ax
0040177E 66250f00 and $0xf,%ax
00401782 0fb7c0 movzwl %ax,%eax
00401785 8b9574ffffff mov -0x8c(%ebp),%edx
0040178B 8a0405f0404100 mov 0x4140f0(,%eax,1),%al
00401792 8802 mov %al,(%edx)

optimization
--------------------------------------------------------------------
WordInsteadOfByteAccess.lpr:40 HexPtr^ := HexDigits[p^ and $F]; // second nibble
0040176A 8b8570ffffff mov -0x90(%ebp),%eax
00401770 660fb600 movzbw (%eax),%ax
00401774 83e00f and $0xf,%eax
00401777 8b9574ffffff mov -0x8c(%ebp),%edx
0040177D 8a80f0404100 mov 0x4140f0(%eax),%al
00401783 8802 mov %al,(%edx)

J. Gareth Moreton

2019-03-07 08:42

developer   ~0114693

Last edited: 2019-03-07 09:21

View 3 revisions

I'll have to check up on that optimisation to see how it slipped our attention for so long.

I would propose another solution though instead of the patch... changing MOVZ to MOV is okay, but for "var15", the size should be changed to S_B, not S_W... and similarly with the other optimisations.

440bx

2019-03-07 16:03

reporter   ~0114700

@Gareth:

"I'll have to check up on that optimisation to see how it slipped our attention for so long."

Given that it usually operates without problem, it's very easy to miss it. I'd say the reason is because the current test cases do not test/use the last accessible byte/word/dword/qword in the range.

Maybe part of the solution is to add (or modify current) test cases to test on that last _accessible_ byte/word/dword/qword as applicable.

Do-wan Kim

2019-03-08 03:51

reporter   ~0114709

I think MOV makes problem in this case.
Changing MOV may make problem in strict size of memory allocation.

J. Gareth Moreton

2019-03-08 16:41

developer   ~0114714

Last edited: 2019-03-08 16:55

View 2 revisions

Created a new test based on the sample code. Unless I've done something wrong, I can't reliably get the access violation to trigger, even though the assembler is bugged - I gather this is because there are guard bytes after the memory block that are being read (and are hence not actually invalid memory).

440bx

2019-03-08 17:12

reporter   ~0114716

Last edited: 2019-03-08 17:18

View 2 revisions

The reason the violation does not trigger reliably is because the code is using GetMem which, more often than not, will allocate a block of memory from the heap which has been carved from free (preallocated and committed) heap memory area. That causes the last byte not to be the last accessible byte, the offending instruction simply reads past the last byte and, most of the time, succeeds because there is more heap memory after it.

The only way that I can think of to reliably reproduce the problem is to use VirtualAlloc and VirtualProtect (and their equivalent for other platforms.) Anything else is unlikely to guarantee that the last byte is also the last accessible byte making the test results unpredictable.

ETA:

if you replace the call to GetMem with VirtualAlloc and FreeMem with VirtualFree, your test will likely work as expected.

J. Gareth Moreton

2019-03-08 17:23

developer   ~0114718

Okay, I'll see what I can do. What would be the equivalent commands for Linux? I would rather make the test as cross-platform as possible.

Cyrax

2019-03-08 17:27

reporter   ~0114719

See attached file WordInsteadOfByteAccess2.zip for examples.

J. Gareth Moreton

2019-03-09 02:59

developer   ~0114725

Last edited: 2019-03-09 03:12

View 2 revisions

Uploaded a new version of the test - how does it perform?

Given that the test appears to complete on earlier versions of FPC, I have a bad feeling that the bug was something I introduced during a bit of refactoring, but was disguised by the fact that it required a very particular set-up for MOVZBW to be emitted, and even then, the internal workings of GetMem masked the buffer overrun. So thank you 440bx for finding the configuration to generate the error.

440bx

2019-03-09 04:28

reporter   ~0114726

Last edited: 2019-03-09 04:29

View 2 revisions

I'm glad I can be of some assistance and, Cyrax is the one that provided Linux examples. Also, whether you are the one who introduced the bug or not, thank you for your efforts to fix it.

About the patch...if I am reading it correctly, I'd say you would be better off having a line "uses Windows" instead of defining the prototypes for VirtualAlloc and VirtualFree and the constants, since those are included in the Windows unit. "uses Windows;" has the added advantage that the test is using the FPC definitions instead of separate definitions which may or may not match those the user will use (since users will be using the Windows unit, not the custom definitions found in the patch.)

therefore, something along the lines of:

{$IFDEF WINDOWS}
  uses
    Windows; // this gives the functions and the constants needed
{$ENDIF}

+{$IFDEF UNIX}
+uses
+ BaseUnix;
+{$ENDIF}
+

seems like a more desirable solution.

Also, instead of having

+{$IFDEF WINDOWS}
+ HeapBlock := VirtualAlloc(nil, 4096, MEM_RESERVE or MEM_COMMIT, PAGE_READWRITE);
+ HeapMarker := HeapBlock + 3968; { 4096 - 128 }
+{$ELSE WINDOWS}
+ {$IFDEF UNIX}
+ HeapBlock := fpmmap(nil, 4096, PROT_READ or PROT_WRITE, MAP_ANON or MAP_PRIVATE, -1, 0);
+ if not Assigned(HeapBlock) then
... etc

something like this:

+{$IFDEF WINDOWS}
+ HeapBlock := VirtualAlloc(nil, 4096, MEM_RESERVE or MEM_COMMIT, PAGE_READWRITE);
{$ENDIF WINDOWS}

+ {$IFDEF UNIX}
+ HeapBlock := fpmmap(nil, 4096, PROT_READ or PROT_WRITE, MAP_ANON or MAP_PRIVATE, -1, 0);
{$ENDIF UNIX}

(conditionally compile _only_ the two memory allocation functions and nothing else)

seems better since, I presume the only difference between the Windows and Linux versions are the system functions used to allocate and free the virtual blocks. Therefore other than the function calls to allocate and free memory, everything else should be the same for both platforms.

Hopefully Cyrax will validate those suggestions and add a few of his own. :-)

J. Gareth Moreton

2019-03-09 04:39

developer   ~0114727

I set it up the way I did so that other platforms can be included if they don't use the Windows or Linux way, especially also since some platforms may have very limited memory and even 4K may be too much.

I probably should have used the Windows unit, but the intention was to make the test as self-contained as possible.

Saying that, I should probably have put tags on the test so it only runs on i8086, i386 and x86_64.

J. Gareth Moreton

2019-03-09 04:59

developer   ~0114728

Is this any better? How does it perform in the test suite?

440bx

2019-03-09 06:24

reporter   ~0114729

Last edited: 2019-03-09 06:41

View 4 revisions

There is one problem in that code. For the test to always work as intended, it must allocate a minimum of 2 pages. The first page is committed and read/write while the second page is not committed (only reserved.) That's the only way I can think of to guarantee that the last byte of the first page is not followed by memory which is readable. If only one page is allocated, then it may happen to be followed by another page not related to the test that may be readable defeating the test's purpose.

What I just stated above cannot happen in Windows because Windows always allocates virtual memory blocks on 64K boundaries, therefore committing a single page will result in 1 page committed followed by 15 pages reserved (and unusable) but, I don't know what alignment other O/Ss apply to virtual blocks. My concern is that other O/Ss such as Linux or some unices may align on a 4k boundary instead. If some do then the possibility exists that the test page could be followed by another page that happens to be readable rendering the test erratic.

I am not familiar with the Linux and Unices memory architectures. Hopefully Cyrax or someone else knowledgeable in that area can shed some light on those.

Personally, I'd play it safe. I'd allocate two pages, one committed and the other one just reserved. That way the test is guaranteed to work as intended.

I understand that for some architectures allocating 8k may be too much. Those architectures may require a customized test.

I think your idea of putting tags to make the test run only on i386 and x86_64 is good. Also, the test could not run on i8086 because protected mode (required for virtual memory) is not available on those processors. The test can only apply to combinations of processors and O/Ss that support virtual memory.

ETA:

I think that your best bet is simply to make some minor modifications to the file Cyrax posted, WordAccessInsteadOfByteAccess2.zip. If you limit the test to apply only to Linux and Windows architectures, that file seems to have everything you need.

I would offer an edited file but, given my extremely limited knowledge of Linux, I am not confident that my offering would work on that platform.

J. Gareth Moreton

2019-03-09 06:57

developer   ~0114730

Give me another hour and it should be ready. Just trying to make it as streamlined as possible and in a format that is consistent with the other tests in the test suite.

It's good that we're at least having a discussion in order to get the test just right. I've got it rewritten now so there are two 4K pages in sequential order... the first one read-write, and the other one no access, since, as you described, it's the only way to be absolutely sure that the memory at the end of the block is invalid.

And as before, there are now two tags in the test:

{ %cpu=i386,x86_64 }
{ %opt=-OoPEEPHOLE }

The first one makes sure that it is only run on i386 and x86_64, while the second one makes sure that the peephole optimizer is enabled, since it was that that was generating the bad machine code.

J. Gareth Moreton

2019-03-09 07:58

developer   ~0114731

Updated.

Also, can you confirm which versions of FPC that this crash occurs on? I was testing it in Lazarus, where it crashed, but I just noticed that I had the 3.0.4 compiler configured, not the trunk.

J. Gareth Moreton

2019-03-09 08:06

developer  

tw35187.patch (3,546 bytes)
Index: tests/webtbs/tw35187.pp
===================================================================
--- tests/webtbs/tw35187.pp	(nonexistent)
+++ tests/webtbs/tw35187.pp	(working copy)
@@ -0,0 +1,112 @@
+program tw35187;
+
+{ %cpu=i386,x86_64 }
+{ %opt=-O1 }
+
+{ NOTE: SIGSEGV won't trigger if GetMem is used because it allocates pages from a large pre-reserved heap. [Kit] }
+
+{$IFDEF TARGET_VALID}
+{$UNDEF TARGET_VALID}
+{$ENDIF TARGET_VALID}
+
+{$IFDEF WINDOWS}
+uses
+  Windows;
+{$DEFINE TARGET_VALID}
+{$ENDIF}
+{$IFDEF UNIX}
+uses
+  BaseUnix, SysCall;
+
+function fpmprotect(Addr: Pointer; Len: PtrUInt; Prot: LongInt): LongInt; inline;
+begin
+  fpmprotect := Do_SysCall(syscall_nr_mprotect, TSysParam(Addr), Len, Prot);
+end;
+{$DEFINE TARGET_VALID}
+{$ENDIF}
+
+{$IFNDEF TARGET_VALID}
+{$ERROR No memory allocation routine available }
+{$ENDIF TARGET_VALID}
+
+const
+  TestBlock: packed array[0..127] of Char = 'The quick brown fox jumps over the lazy dog, Victor jagt zw'#148'lf Boxk'#132'mpfer quer '#129'ber den gro'#225'en Sylter Deich.'#10#13'0123456789?!"#%'#251#253#252;
+
+  Expected: packed array[0..255] of Char = '54686520717569636B2062726F776E20666F78206A756D7073206F76657220746865206C617A7920646F672C20566963746F72206A616774207A77946C6620426F786B846D70666572207175657220816265722064656E2067726FE1656E2053796C7465722044656963682E0A0D303132333435363738393F21222325FBFDFC';
+
+  HexDigits: packed array[0..$F] of Char = '0123456789ABCDEF';
+
+var
+  Buf: packed array[0..255] of Char;
+  HexPtr: PChar;
+  P: PByte;
+  I: DWord;
+  HeapBlock, HeapMarker: PByte;
+begin
+  WriteLn(TestBlock);
+  FillChar(Buf, SizeOf(Buf), 0);
+
+  { Reserve two 4K memory pages: one that is read-write followed by one that
+    has no access rights at all and will trigger SIGSEGV if encroached }
+{$IFDEF WINDOWS}
+  HeapBlock := VirtualAlloc(
+                 VirtualAlloc(nil, 8192, MEM_RESERVE, PAGE_READWRITE),
+                 4096,
+                 MEM_COMMIT,
+                 PAGE_READWRITE
+               );
+  if not Assigned(HeapBlock) then
+    begin
+      WriteLn('Memory allocation failure');
+      Halt(1);
+    end;
+  HeapMarker := HeapBlock + 3968; { 4096 - 128 }
+{$ENDIF WINDOWS}
+{$IFDEF UNIX}
+  HeapBlock := fpmmap(nil, 8192, PROT_NONE, MAP_ANON or MAP_PRIVATE, -1, 0);
+  if not Assigned(HeapBlock) or (fpmprotect(HeapBlock, 4096, PROT_READ or PROT_WRITE) <> 0) then
+    begin
+      WriteLn('Memory allocation failure');
+      Halt(1);
+    end;
+
+  HeapMarker := HeapBlock + 3968; { 4096 - 128 }
+{$ENDIF UNIX}
+
+  Move(TestBlock, HeapMarker^, SizeOf(TestBlock));
+  HexPtr := @Buf;
+
+  for I := 0 to SizeOf(TestBlock) - 1 do
+  begin
+    P := HeapMarker + I;
+
+    HexPtr^ := HexDigits[P^ shr 4]; { first nybble }
+    Write(HexPtr^);
+    Inc(HexPtr);
+
+    { #35187: This instruction causes an access violation on the last byte
+        because it tries to read a word instead of a byte. }
+
+    HexPtr^ := HexDigits[P^ and $F]; { second nybble }
+    Write(HexPtr^);
+    Inc(HexPtr);
+  end;
+
+{$IFDEF WINDOWS}
+  VirtualFree(HeapBlock, 0, MEM_RELEASE);
+{$ENDIF WINDOWS}
+{$IFDEF UNIX}
+  fpmunmap(HeapBlock, 8192);
+{$ENDIF UNIX}
+
+  WriteLn();
+
+  for I := 0 to SizeOf(TestBlock) - 1 do
+    if Buf[I] <> Expected[I] then
+    begin
+      WriteLn('Error at index ', I, '; expected ', Expected[I], ' got ', Buf[I]);
+      Halt(1);
+    end;
+
+  WriteLn('ok');
+end.
tw35187.patch (3,546 bytes)

J. Gareth Moreton

2019-03-09 08:07

developer   ~0114732

Confirmed the crash also occurs on the trunk, but only if -O1 is selected. Just having -OoPEEPHOLE won't trigger the bug. (Updated test to change the %opt setting)

Florian

2019-03-10 11:49

administrator   ~0114765

Thanks for the patch, applied in r41667.

Issue History

Date Modified Username Field Change
2019-03-05 01:26 440bx New Issue
2019-03-05 01:26 440bx File Added: WordInsteadOfByteAccess.7z
2019-03-05 10:57 Cyrax File Added: WordInsteadOfByteAccess.zip
2019-03-05 10:58 Cyrax Note Added: 0114653
2019-03-05 10:58 Cyrax Note Edited: 0114653 View Revisions
2019-03-05 11:03 Cyrax Note Added: 0114654
2019-03-05 11:05 Cyrax File Added: WordInsteadOfByteAccess.s_i386-linux.zip
2019-03-05 11:06 Cyrax Note Added: 0114655
2019-03-05 11:09 Cyrax File Added: fpc-debug-output-and-tree-dump_i386-linux.zip
2019-03-05 11:15 Cyrax Note Added: 0114656
2019-03-05 11:17 Cyrax File Added: WordInsteadOfByteAccess.s_i386-win32.zip
2019-03-05 11:28 Cyrax File Added: WordInsteadOfByteAccess.s_x86_64-linux.zip
2019-03-05 11:31 Cyrax File Added: WordInsteadOfByteAccess.s_x86_64-win64.zip
2019-03-05 11:38 Cyrax File Added: WordInsteadOfByteAccess2.zip
2019-03-05 11:38 Cyrax Note Added: 0114657
2019-03-05 14:05 Cyrax Note Added: 0114661
2019-03-05 19:27 Jeppe Johansen Note Added: 0114668
2019-03-07 05:22 Do-wan Kim File Added: aoptx86.pas-35187.patch
2019-03-07 05:25 Do-wan Kim Note Added: 0114692
2019-03-07 06:49 Do-wan Kim Note Edited: 0114692 View Revisions
2019-03-07 07:30 Do-wan Kim Note Edited: 0114692 View Revisions
2019-03-07 08:42 J. Gareth Moreton Note Added: 0114693
2019-03-07 09:17 J. Gareth Moreton Note Edited: 0114693 View Revisions
2019-03-07 09:21 J. Gareth Moreton Note Edited: 0114693 View Revisions
2019-03-07 16:03 440bx Note Added: 0114700
2019-03-08 03:51 Do-wan Kim Note Added: 0114709
2019-03-08 16:41 J. Gareth Moreton File Added: tw35187.patch
2019-03-08 16:41 J. Gareth Moreton Note Added: 0114714
2019-03-08 16:47 J. Gareth Moreton File Deleted: tw35187.patch
2019-03-08 16:48 J. Gareth Moreton File Added: tw35187.patch
2019-03-08 16:53 J. Gareth Moreton File Deleted: tw35187.patch
2019-03-08 16:53 J. Gareth Moreton File Added: tw35187.patch
2019-03-08 16:55 J. Gareth Moreton Note Edited: 0114714 View Revisions
2019-03-08 17:12 440bx Note Added: 0114716
2019-03-08 17:18 440bx Note Edited: 0114716 View Revisions
2019-03-08 17:23 J. Gareth Moreton Note Added: 0114718
2019-03-08 17:27 Cyrax Note Added: 0114719
2019-03-09 02:52 J. Gareth Moreton File Deleted: tw35187.patch
2019-03-09 02:52 J. Gareth Moreton File Added: tw35187.patch
2019-03-09 02:55 J. Gareth Moreton File Deleted: tw35187.patch
2019-03-09 02:56 J. Gareth Moreton File Added: tw35187.patch
2019-03-09 02:59 J. Gareth Moreton Note Added: 0114725
2019-03-09 03:12 J. Gareth Moreton Note Edited: 0114725 View Revisions
2019-03-09 04:28 440bx Note Added: 0114726
2019-03-09 04:29 440bx Note Edited: 0114726 View Revisions
2019-03-09 04:39 J. Gareth Moreton Note Added: 0114727
2019-03-09 04:58 J. Gareth Moreton File Deleted: tw35187.patch
2019-03-09 04:58 J. Gareth Moreton File Added: tw35187.patch
2019-03-09 04:59 J. Gareth Moreton Note Added: 0114728
2019-03-09 05:01 J. Gareth Moreton Tag Attached: compiler
2019-03-09 05:01 J. Gareth Moreton Tag Attached: optimization
2019-03-09 05:17 J. Gareth Moreton Priority normal => high
2019-03-09 05:17 J. Gareth Moreton Severity minor => crash
2019-03-09 06:24 440bx Note Added: 0114729
2019-03-09 06:38 440bx Note Edited: 0114729 View Revisions
2019-03-09 06:39 440bx Note Edited: 0114729 View Revisions
2019-03-09 06:41 440bx Note Edited: 0114729 View Revisions
2019-03-09 06:57 J. Gareth Moreton Note Added: 0114730
2019-03-09 07:54 J. Gareth Moreton File Deleted: tw35187.patch
2019-03-09 07:54 J. Gareth Moreton File Added: tw35187.patch
2019-03-09 07:58 J. Gareth Moreton Note Added: 0114731
2019-03-09 08:06 J. Gareth Moreton File Deleted: tw35187.patch
2019-03-09 08:06 J. Gareth Moreton File Added: tw35187.patch
2019-03-09 08:07 J. Gareth Moreton Note Added: 0114732
2019-03-10 11:49 Florian Fixed in Revision => 41667
2019-03-10 11:49 Florian Note Added: 0114765
2019-03-10 11:49 Florian Status new => resolved
2019-03-10 11:49 Florian Fixed in Version => 3.3.1
2019-03-10 11:49 Florian Resolution open => fixed
2019-03-10 11:49 Florian Assigned To => Florian