Re: Reading PNG Blob images causes access violation on png.c

On Tue, 30 Oct 2018, Antti Luoma wrote:

> Hi,
>
> 1) I haven't tried this actually very much without our memory handling.
> First when I today tried I got system abort() call from Magick as Magick
> signature was 0, which was strange ( I had few questionable code lines also
> so maybe that has something to do with it.). But after using the default
> memory allocations I was actually able to read PNG blob without access
> violation. This is really good news as I have tried quite many things
> already but not this it seems. I should also note that reading jpg/bmb/tiff
> pictures works with our memory routines.

As a safeguard, the signature fields are set to zero when a structure 
is freed.  If there is an assertion due to a value of zero, that 
implies use of a structure which was already freed.

> Now I am wondering what in our memory routines is causing this access
> violation. Only thing which makes me wonder most is the fact that we use
> our session pointer as a context variable. We store this variable in
> ThreadLocalStorage and retrieve it from there (TlsGetValue()) before memory
> allocation as usually memory is pre-allocated and we just mark it as
> reserved. This is my guess that what could go wrong here but I don't know
> if I have understood setjmp/longjmp enough.

If it helps, the allocator that GraphicsMagick uses by default is 
based on the standard C library's malloc()/free() ("heap memory"). 
This helps when exchanging a buffer with some other library which 
expects that a buffer was allocated using malloc().  Windows does 
provide ways to allocate memory besides malloc().

I have heard that when DLLs are used that memory allocated by a DLL 
must be freed by the same DLL.

Setjmp/longjump works by restoring thread stack memory and registers 
to the values which were present when setjmp() is called.  It is 
pretty dangerous but it its use is demanded by libpng and libjpeg 
other than using a worker thread and thread cancellation, or a true 
exception mechanism (similar to C++).  It may be that use of 
setjmp/longjmp with threads is safer than what was originally assumed 
in GraphicsMagick but there are still dangers.

> 2) When reading png images the semaphore that was not released was the
> png.c own static one. There was other places as well but I have to get back
> on this later and be more specific.

We need to find out the exact exception case which resulted in this 
problem.  The png.c code is very complex and a great many problems 
have been fixed already with releasing resources when an error is 
reported.  The semaphore would be a resource to be released just like 
memory, or closing libpng handles.

> I also tried to define SETJMP_IS_THREAD_SAFE some day but I got some
> concurrency errors but those could also be related to the setjmp issue so I
> will have continue to work on this and I will report if there are issues
> that should checked. I have configured logging for stdout.

Exploring this issue is interesting to me.  The semaphore 
single-threads use of libpng as well as protecting against any 
potential harm due to setjmp/longjmp.

> We use checks/guards for our own semaphores so that if exception happens in
> some client session we check are all the semaphores released for this
> session (with a session id) and try to release those so that we don't cause
> permanent locks for shared resources.

It sounds like you may be using C++ where automated cleanup like this 
is possible.

Bob
-- 
Bob Friesenhahn
[email protected], http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/