Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aremmell
Copy link

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:

  • GFXCanvas1::drawFastRawVLine
  • GFXCanvas1::drawFastRawHLine
  • GFXcanvas8::drawFastRawVLine
  • GFXcanvas8::drawFastRawHLine
  • GFXcanvas16::drawFastRawVLine
  • GFXcanvas16::drawFastRawHLine

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
Copy link
Contributor

BillyDonahue commented Dec 12, 2023

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants