[BUG] HTTPS support in 64bits - Access Violation

[Guest]

You still confused and have two different misunderstanding in two different places,

1) You need to understand the difference between Pointer and TBytes , you can cast pointer into TBytes and it is very safe as long you don't try to get its length or resize it,

This :

A:Pointer;

A:=GetMemory(100);

SetLegnth(TBytes(A)); // Wrong

Length(TBytes(A));  // wrong

 

access an item (byte) in a cast is completely safe

A:=GetMemory(100);

TBytes(A)[13]:=77; // works fine as long A points to data (buffer) where the Byte with index=13 is safe to write to memory here A hold 100 bytes

so in IcsWireFmtToStrList have Len as parameter which should be safe as returned from OpenSSL callback in inLen

 

2) your second problem is with Move the third parameter is count in bytes to be copied so copy like this

9 hours ago, Chris Rolliston said:

Move (Buffer [offset], AStr [1], mylen * SizeOf (TBytes));

and from this

20 hours ago, Kas Ob. said:

(2) 104 50 (8) 104 116 116 112 47 49 46 49

This is equivalent to "2h28http/1.1"

mylen for h2 will be 2 and the number of bytes to be copied will be !!!???

2* 4 =8  on 32bit

2*16 =16 on 64bit

while the protocol name must be 2 chars , (2 ansichars in fact ) = 2 bytes and those will be 4 bytes in string item in SList

 

Do you get it now ?

 

Please follow this link and get better understanding on two very important things there, Strings and Dynamic arrays 

http://docwiki.embarcadero.com/RADStudio/Rio/en/Internal_Data_Formats_(Delphi) 

[Angus Robertson 574]

His solution may have fixed the exception, but will not return any sensible ALPN data, and will probably introduce other problems instead. 

 

I did wonder whether I should use PAnsiChar instead of TBytes, which would be safer in case someone in the future tried to resize the parameter.  But TBytes seems to be the preferred method for working with binary and is mobile compatible, where Ansi is not.

 

Angus

 

[Guest]

In the past i leaned on PAnsiChar but for years now i stick to TBytes or PByte in such cases,

As for fixed his problem or not, i am sure it didn't if no exception was raised then it is simple luck,

He didn't answer the questions about what exactly he is getting in IcsWireFmtToStrList parameters, i was interested if a special build of OpenSSL he is using might be buggy.

[Marcelo Jaloto 1]

@Angus Robertson

 

I installed version 8.64 which was on SVN as recommended and the access violation error keeps happening in the same location.

 

 

Adding the code lines of my solution and its changes the problem no longer occurs in the IcsWireFmtToStrList method and same result is obtained in 32 and 64 bits.

 

However in other locations other new Access Violation issues occur using 64-bit using version 8.64 that were not occurring in version 8.63. I don't know what caused them after their changes.

 

After that I followed his instructions on OverbyteIcsSslWebServ sample using version 8.64. And there are also Access Violation issues when loading demo.html.

 

So as my last solution was not accepted I went back to my first palliative solution by forcing the decision to http / 1.1 and commenting on the calls to IcsWireFmtToStrList in the AlpnSelectCallBack method as first post and back to version 8.63.

 

I will wait for a definitive 64bits solution of yours that will work with HTTPS support.

[Guest]

@Marcelo Jaloto Great screenshot only , would you please recapture it with project Optimization disabled ? and one more screenshot on the line with while when the exception will raise following that break

[Angus Robertson 574]

Since I can not reproduce the problem with Win64, I'll do nothing more for now.  I still await the IcsLogger log lines I asked for.  

 

Angus

 

[FPiette 383]

Using madExcept is of great help regarding accessing invalid memory, buffer overflow, freed memory or object and so on. I recommand using it for such issue.

 

[Guest]

I think i found the problem 

 

function IcsWireFmtToStrList(Buffer: TBytes; Len: Integer; SList: TStrings): Integer;

Should be declared with const or var Buffer

 

function IcsWireFmtToStrList(const Buffer: TBytes; Len: Integer; SList: TStrings): Integer;

or
function IcsWireFmtToStrList(var Buffer: TBytes; Len: Integer; SList: TStrings): Integer;

as the compiler is changing the reference,

 

@Marcelo Jaloto Would you try this ?

[Marcelo Jaloto 1]

@Kas Ob.

The problem can be reproduced by following @Angus Robertson instructions with the example OverbyteIcsSslWebServ.
I suggest going back to version 8.63 to resolve this issue and only then moving on to version 8.64 because you have several other Access Violation issues.

 

You may not have seen it in one of my posts regarding parameter values.

 

The Len parameter comes with the value 12.

The first 12 characters in ordinal form in the Buffer parameter byte array are:

2-104-50-8-104-116-116-112-47-49-46-49

ignore the "-" above. just for ease of understanding.

This is equivalent to "2h28http / 1.1"

 

The result in SList is:
h2
http / 1.1

 

@FPiette 

here we use madException. All my tests were done with madException.

[Guest]

@Marcelo Jaloto Try const with function IcsWireFmtToStrList(const Buffer: TBytes;  Len: Integer; SList: TStrings): Integer;

 

for that i always use const or var with all managed type in parameters here is the bug in action

 

[Marcelo Jaloto 1]

@Kas Ob.

Your solution is working without my changes in version 8.63.

I used const to make it more appropriate.

function IcsWireFmtToStrList (const Buffer: TBytes; Len: Integer; SList: TStrings): Integer;

 

Edited January 7, 2020 by Marcelo Jaloto

[Guest]

@Marcelo JalotoThank you!

[Guest]

@Marcelo JalotoNo matter how much one think he knows, one can miss things like this, i advised you to read more about Dynamic array and myself missed that, so read more and hope to remember, i fully aware of stack corruption when casting managed types, yet managed to miss it the whole thread.

[Marcelo Jaloto 1]

@Angus Robertson

Could you review the @kas solution? And add it in the next version?

Remember that in version 8.64 there are still other problems that may be solved with this same solution.

In the OverbyteIcsSslWebServ example I noticed that I didn't have the Win64 platform added to the project. IOS Device 64bits only and Win 32bits.

I don't understand why you couldn't reproduce the problem if it happens to me so easily without any of the solutions. Maybe you have different DLLs from my other required file?

[Guest]

For me i don't have ICS library installed, you can't ask Angus or anyone why he didn't found it, you must understand that overwritten stack is the hardest bug to track and fix, like this if it didn't land on something critical then no exception will be raised, and this is simple luck nothing more, either you can find it in first moment or will cause you headache and time to catch it later.

Also Madshi or EurekaLog can't do anything about it as they don't see it, on contrary they might send you in wrong direction.

[Marcelo Jaloto 1]

It is important to understand what has been documented here. For future similar problems you can use this as a lesson learned. I understand that the problem is hard to find and I also understand about dynamic array, but thanks for the readability indicated. The fact that your solution also worked does not mean that it is the only acceptable solution and that I cannot question some other factors. What it seems to me is that there are other related issues compiling with 64 bits.
And most important of all is we are working as a team and solving the problems that are coming up.

[Angus Robertson 574]

Making it const Buffer: TBytes; is a safe change, var would be dangerous since it's a cast pointer. 

 

Still confused why it would be necessary, I always thought pointers did not need to be declared const, and the different behaviour when compiled with Win32 and Win64 suggests a compiler bug to me?  

 

I'll need to check a few other functions using TBytes.

 

Angus

 

[Guest]

Var and Const are safe to be used like that, the problem is not with pointers but with with TBytes and managed types, both declaration will cause the compiler to skip ability to use the parameter as local variable, if you looked at the assembly code you will find without const or var that the compiler added 

call @DynArrayAddRef 

and

call @DynArrayClear

 

to handle the reference counter like it was locally declared, as you see in my last screenshots how the bug manifested in Marcelo case and not with you, in his case by luck the reference counter held a value than 0 which was at position -12 ( that is minus ) on 64bit ,so DynArrayAddRef increased it to 1 and then DynArrayClear decreased it again to 0, and because it is managed with 0 reference counter it did tried to free the memory that is starting at this -12 from the pointer and raised the access violation, may be it is on stack memory belongs to OpenSSL or a buffer allocated by OpenSSL but diffidently not the Application MM.

Adding const or var makes the compiler skip this reference handling and makes it safe, in the screenshot you can see the value 10 instead of 9 in different position (-8) and (-12), the exception is hidden as long those position are accessible and don't have 0 value. 

Edited January 8, 2020 by Guest

[Guest]

I think the only way to capture such bug may be by using Pascal Analyzer, my license long time expired and didn't use it for years now, but foggily i remember it does warns about something like that, not sure though.

If not it can be extended with such warning easily i think.

 

One might consider this as free advertising for great Delphi tool,... yes it is.

[Angus Robertson 574]

So reference counting was the culprit, sorry missed that in the blizzard of overnight messages.  I don't use TBytes very often but will carefully check ICS for all such use.  As I said yesterday, the web server sample was erratic under Win64, but my development PC had not been rebooted since the last patch Tuesday and does become more unstable over time.  Should be able to update SVN later today.

 

Angus

 

[Guest]

I depend on TBytes in most cases like the above but i really don't change the passed parameter in other words i rarely cast it, while i heavily use absolute vars declaration, like in every VCL events when i need to access Sender i declare a shadow with absolute type to it, such declaration is friendly with compiler as it does not try to manage it, and here you can pass it as pointer as it passed on by OpenSSL callback and then use an absolute var.

The advantage of using absolute that you can declare many vars points to the same location, this will make the code shorter and more readable, like a pointer been passed and you can access it by TBytes and PUInt32 and PUInt16 at the same time, without casting.

[Angus Robertson 574]

SVN and the overnight zip are updated with the 64-bit fix.  Still having fun and games with 4-bit samples, think I'll try an older version of Delphi. 

 

I checked other functions with TBytes, most already had const or var, a couple were missing both so added, although they were only used in ICS with TBytes buffers so should not have caused any issues.

 

Angus