fix crash by properly propagating error condition (out of heap memory / image too large)

[GerHobbelt]

Fix: as it says on the tin.

Context / background info

I ran into this one while testing a customized tesseract/leptonica combo, where I had made a mistake, which resulted in all JPEG and JP2 files failing to load because those libraries invoked a customized malloc/free wrapper, which just b0rked by returning NULL for every malloc attempt.

The original intent was that it SHOULD only do so for "ridiculously large malloc request sizes" but then I added an embarrassing primary school level calculus mistake and that made everything deemed "ridiculously oversized", hence the consistent NULL-only produce there. 😊 😰

Thus the result is pix being NULL. leptonica has by now already reported something went wrong with the image memory allocation, but once it got here, pix == NULL, this caused a crash because the next few API methods were invoked with that NULLed pix and that didn't go down well. 💣 💥

The added check fixes this by preventing NULL pointer dereferencing in this part of leptonica and properly returning to caller as quick as possible. With an additional error report added in for good measure. (In my case, when the custom malloc/free wrapper was still buggered, my code would produce a chain of about 5 leptonica error messages (from different spots in the call chain), before finally calling it quits and terminating in an orderly fashion. Which, AFAIAC, is nice behaviour. The number of error report lines might be reduced if you're nitpicking, but that is really very much UNimportant from my point of view; fixing the crash is important, however.

[DanBloomberg]

This is puzzling. Admittedly,

pixSetInputFormat(pix, IFF_JFIF_JPEG);

should happen after the test for pix == NULL, but it still should not cause a problem: if pix is NULL, it just returns an error message.

Note that after that, if either rowbuffer or pix is NULL, the function cleans up allocated memory and returns. Your code does not clean up memory.

So the question for you is this: if you move pixSetInputFormat() to after the test in line 341, is there a crash?

[DanBloomberg]

There is a slight chance that the problem occurs in pixCreateHeader(), where a very small allocation for a pix struct is not checked. This is l. 542 of pix1.c.

If you're still getting a crash after making the suggestion above, try testing for pixd there, and if the calloc fails, return NULL.

[GerHobbelt]

currently AFK (dev machine).

but I do recall those debugger stack traces listing pixcreateheader as the "culprit" at the time I was looking into this.

so that's a bingo!

On Thu, Mar 9, 2023, 06:12 Dan Bloomberg @.***> wrote:

There is a slight chance that the problem occurs in pixCreateHeader(), where a very small allocation for a pix struct is not checked. This is l. 542 of pix1.c.

If you're still getting a crash after making the suggestion above, try testing for pixd there, and if the calloc fails, return NULL.

— Reply to this email directly, view it on GitHub https://github.com/DanBloomberg/leptonica/pull/676#issuecomment-1461289350, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADCIHQICAWCU4CPJTFBWATW3FRDRANCNFSM6AAAAAAVULFZVY . You are receiving this because you authored the thread.Message ID: @.***>

[DanBloomberg]

Pushed a change that should prevent the crash. Please test.

[GerHobbelt]

Pulled & merged your latest into my repo. Tested OK! 👍

AFAIAC this successfully closes this issue/pullreq and obsoletes my suggested fix, removing the need for this pullreq.

Thank you!

[DanBloomberg]

You're welcome. Glad it was an easy fix.