[prev in list] [next in list] [prev in thread] [next in thread] 

List:       squeak-vm-dev
Subject:    Re: [Vm-dev] Possible bug in FFICallbackThunk class>>#freeBlockInExecutablePage:
From:       Nicolas Cellier <nicolas.cellier.aka.nice () gmail ! com>
Date:       2020-07-08 21:42:39
Message-ID: CAKnRiT4ZtnsuwEDvBWNJ5w=2na2q9xT5V+zbJjCbqjej3PFgrg () mail ! gmail ! com
[Download RAW message or body]

[Attachment #2 (text/plain)]

 
[Attachment #3 (multipart/alternative)]


Le mer. 8 juil. 2020 Ã  21:13, Bob Westergaard <bwestergaard@gmail.com> a
écrit :

>
> Hi,
>
> I believe there is a bug in FFICallbackThunk
> class>>#freeBlockInExecutablePage: (in Alien-Core).
> If there are multiple pages allocated, it is possible that the address
> comparison with the end of  the executable page will mistakenly pass.  What
> will end up happening is that you'll get an #unsignedByteAt:put: failure
> ('bad index') error during finalization when the section of that page is
> being marked as free.
>
> For example, I saw the failure where the address being freed was 1785856
> (16r1B4000).  There was a page at 16r1B3000, so the check for this address
> evaluates to true.  This ends up trying to use 4097 as the index (dataSize
> is 4096 here).
>
>     (address >= alienAddress
>     and: [alienAddress + alienPage dataSize >= address]) ifTrue:
>         [alienPage unsignedByteAt: address - alienAddress + 1 put: 0.
>     ...
>
> The newest version I found on squeak source was Alien-Core-nice.104 and
> the test is the same.
>
> I changed >= to > here and the problem went away for me:
>
>     (address >= alienAddress
>      and: [alienAddress + alienPage dataSize > address]) ifTrue:
>
> Is my reading of this correct?
>
> yes, if dataSize were 1, then we would have a single valid address in the
page, alienAddress <= address < alienAdress + 1
so I think that you are right.

Since we are here, why do #initializeX64 and #initializeX64Win64 end their
> methods with sending #primThunkEntryAddress and doing nothing with the
> returned result? Is there a side-effect going on here?  It looks to me like
> a possible copy and paste error.
>
> I don't see any side effect here, you can safely remove.
For initializeX64Win64 I just did mimic the existing initializeX64
(Alien-nice.36) without questioning further...

Regards,
> -- Bob
>

[Attachment #6 (text/html)]

<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" \
class="gmail_attr">Le  mer. 8 juil. 2020 Ã   21:13, Bob Westergaard &lt;<a \
href="mailto:bwestergaard@gmail.com">bwestergaard@gmail.com</a>&gt; a écrit  \
:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">  <div \
dir="ltr">Hi,<div><br><div>I believe there is a bug in FFICallbackThunk \
class&gt;&gt;#freeBlockInExecutablePage: (in Alien-Core).  </div><div>If there are \
multiple  pages allocated, it is possible that the address comparison with the end of \
the executable page will mistakenly pass.   What will end up happening  is that \
you&#39;ll get an #unsignedByteAt:put: failure (&#39;bad index&#39;) error during \
finalization when the section of that page is being marked as \
free.</div><div><br></div><div>For example, I saw the failure where the address being \
freed was  1785856 (16r1B4000).   There was a page at 16r1B3000, so the check for \
this address evaluates to true.   This ends up trying to use 4097 as the index \
(dataSize is 4096 here).</div>





<div><br></div><div>      (address &gt;= alienAddress</div>      and: [alienAddress + \
alienPage dataSize &gt;= address]) ifTrue:<div>            [alienPage unsignedByteAt: \
address - alienAddress + 1 put: 0.<br></div><div>      \
...</div><div><br></div><div><div>The newest version I found on squeak source was  \
Alien-Core-nice.104 and the test is the \
same.</div><div></div></div><div><br></div><div>I changed &gt;= to &gt; here and the \
problem went away for me:</div><br>      (address &gt;= alienAddress<br>        and: \
[alienAddress + alienPage dataSize &gt; address]) ifTrue:  <div><br></div><div>Is my \
reading of this correct?</div><div><br></div></div></div></blockquote><div>yes, if \
dataSize were 1, then we would have a single valid address in the page, alienAddress \
&lt;= address &lt; alienAdress  + 1</div><div>so I think that you are \
right.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div \
dir="ltr"><div><div></div><div>Since we are here, why do #initializeX64 and \
#initializeX64Win64 end their methods with sending #primThunkEntryAddress and doing \
nothing with the returned result? Is there a side-effect going on here?   It looks to \
me like a possible copy and paste \
error.</div><div><br></div></div></div></blockquote><div><div>I don&#39;t see any \
side effect here, you can safely remove.</div>For initializeX64Win64 I just did mimic \
the existing initializeX64 (Alien-nice.36) without questioning \
further...<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px \
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div \
dir="ltr"><div><div></div><div>Regards,</div><div>-- Bob</div></div></div> \
</blockquote></div></div>



[prev in list] [next in list] [prev in thread] [next in thread]