adafruit / Adafruit-GFX-Library

Adafruit GFX graphics core Arduino library, this is the 'core' class that all our other graphics libraries derive from
https://learn.adafruit.com/adafruit-gfx-graphics-library
Other
2.41k stars 1.55k forks source link

BUGFIX: null pointer access violations in Canvas1/8/16 when buffer is not successfully malloc'd #441

Open aremmell opened 11 months ago

aremmell commented 11 months ago

Most of the methods in the Canvas classes check that buffer is not null before doing any memory I/O based on its address, but some were overlooked. If malloc fails in the constructor, the following methods will result in an access violation:

These methods are called by commonly used methods, such as fillRect, so this is a likely scenario. In particular, large displays are problematic; the one I'm using is 800x480, so it is attempting to allocate 800 480 2 = 768 KB of RAM, and my poor little ESP32-S3 Feather has around half of that onboard.

With the proposed changes in this PR, the canvas still won't function, obviously, but at least it won't cause boards to spontaneously reset with no clue as to why.

In addition to the above, I have also taken the liberty to add an isValid virtual method to Adafruit_GFX so that any subclass which manages a frame buffer or other properties that may fail to be initialized properly may implement it. This provides the caller with a viable means of testing whether or not they should even attempt to draw using a given object. I have already implemented it in the Canvas classes.

I do realize this may not be received well due to the sheer number of derived classes that Adafruit_GFX has, but the default implementation returns true, so subclasses that do not implement it will have zero change in effective behavior (as they already have no way to check if they are usable at runtime).

I reckon that subclasses which should implement it can be dealt with on a case-by-case basis, whenever the opportunity arises in the future.

Thank you for the fantastic libraries and hardware products. Keep 'em coming!

BillyDonahue commented 11 months ago

I think adding an isValid function is an ok compromise for an object that can fail, on a platform without exceptions. This can help apps look before they leap. But checking the validity of buffer before every line of code that that touches the buffer is IMO wasted branches and a maintenance burden (as this PR points out, buffer isn't uniformly checked).

With those ifs in place, the user will still have a failed object and ultimately a failed app, it will just mysteriously do nothing instead of mysteriously crashing.

Alternative: when the buffer is allocated, perhaps there should be one assertion that it's valid, and then the rest of the code can safely proceed leveraging that assumption.

aremmell commented 11 months ago

I don't disagree with that assessment. I would prefer an assertion or a C++ exception that terminates the program early rather than running in a broken (and irreparable) state. That being said, I don't know what you guys' style and policy is about such things, so I just threw something together that would let me be able to test if it failed without a mysterious reset loop.

aremmell commented 5 months ago

Anything? I believe this is better than a reset loop that a noob may not be able to diagnose while trying to use these classes... If you don't like the NULL check, what about assertions?