-1

This may be a total obvious error for some, but I can't seem to find out why this segmentation fault happens.

I do understand that segmentation faults occur when accessing an address that my program should not access.

First, I have this base-class called "UiObject":

class UiObject {
  private:

  public:
    UiObject();
    virtual void render(char * imageBuffer) = 0;
};

Then, I am deriving multiple other classes from the UiObject class: For example, the ProgressBar class:

class ProgressBar : public UiObject {
    private:
        int x,y; // Position der ProgressBar auf dem Bildschirm (oben links) (Pixeln)
        int width,height; // Breite und Höhe in Pixeln
    public:
        int maxProgress; // Maximal zu erreichner Fortschritt
        long progress; // Fortschritt aktuell
        ProgressBar(); // Constructor
        void render(char * framebuffer); // Rendermethode
};

Now, these objects are managed and rendered by an UiManager. The function renderDisplay() is a thread created with std::thread:

void UiManager::renderDisplays() {
  printf("[RENDER] Render thread has been started\n[RENDER] (%x)\n", this);
  while(!this->shouldStop) {
    // Removed code that manages the timing of this function (running approximately 60FPS)
  
    // Rendering:

    for(Display d: displayArray) {
      char * framebuffer = d.getFrameBuffer();
      printf("Framebuffer: %x\n", framebuffer);
      printf("uiArray: %x\n", uiArray);
      printf("&uiArray: %x\n", &uiArray);
      for(UiObject* pObject : uiArray) {
        printf("[RENDER] (%x) Rendering Object...", pObject);
        pObject->render(framebuffer);
      } 
      d.renderDisplay();
    }
  }
}

As you can see, for every added Display I am retrieving the framebuffer. I'm opening the /dev/fbX files so i can draw directly onto the screen. This all works fine. d.getFramebuffer() returns a char * pointer so i can manipulate the pixels, which is then passed into the render(char * framebuffer) function of any UiObject.

However, this function call is creating a segfault, and I can't understand why. There are sometimes when it works and just runs until the thread should stop, and sometimes it crashes immidiately after the first render-function call.

I add the UiObjects to the UiManager using this:

 ProgressBar pBar;
 pBar.maxProgress = 60*15;
 pBar.progress = 1;

 uiArray.push_back(&pBar);

The UiManager has this as its class-definition:

  class UiManager {
  private:
    char ttyfd; // TTY File Descriptor.
    std::thread uiManagerThread;
    std::chrono::system_clock::time_point timeA;
    std::chrono::system_clock::time_point timeB;
    std::chrono::duration<double, std::milli> work_time;

  public:
    std::vector<Display> displayArray;
    std::vector<UiObject*> uiArray;
    bool shouldStop;
    bool displayFPS;

    UiManager();

    void stop();

    void renderDisplays();

    void addDisplay(Display d);

    void startThread();

    void stopThread();
};

Now I'm wondering why this is.

According to the cppreference https://en.cppreference.com/w/cpp/language/abstract_class and their documentation about abstract classes, I'm not allowed to have the =0; in my UiObject class. If I remove it, I will get compiler warnings during linking with the message "undefined reference to vtable for UiObject".

What have I done wrong?

I suspect that the std::vector and the for-loop are not quite working in the UiManager.

Im using g++ on debian as my compiler.

My console output:

[RENDER] Render thread has been started
[RENDER] (6ea609f0)
Framebuffer: e09f0010
uiArray: e09eed20
&uiArray: 6ea60a30
./run.sh: Zeile 21: 25018 Speicherzugriffsfehler  ./a.out

It also does not even jump into the ProgressBar render routine. I also commented out the contents of the render-function, therefore i can only suspect that specific function call.

Gandalf1783
  • 11
  • 1
  • 8
  • Have you checked your compiler warnings? – littleadv Jan 29 '22 at 15:53
  • Actually, there is only one compiler warning that is not related to this issue. Maybe i have to enable them specifically? The one is a conversion from a string to char * that is done automatically... – Gandalf1783 Jan 29 '22 at 17:57

2 Answers2

2

= 0 for an abstract base class is correct.


However,

 ProgressBar pBar;
 //...
 uiArray.push_back(&pBar);

is not.

pBar will be destroyed once the scope in which it is declared is left and then trying to dereference the dangling pointer later in renderDisplays is undefined behavior.

You need to create the object with new, or better store std::unique_ptr<UiObject> in the vector and create objects with std::make_unique<ProgressBar>().

Also, UiObject needs a virtual destructor: virtual ~UiObject() = default; Otherwise destroying objects through the base class pointer will cause undefined behavior.


Then there seems to be also no synchronization between the access to uiArray in the two threads.

Accessing uiArray for modification and read in two threads without synchronization is a data race and undefined behavior.

You need to add a mutex and lock it when accessing uiArray in both threads.

user17732522
  • 53,019
  • 2
  • 56
  • 105
  • Using your answer and the one from Grifball I was able to get the code working without any segfaults. What kind of undefined behaviour are you referring to? Not freeing the space of the class? What would be the consequences? Also, thanks for mentioning the keyword mutex. I currently only add objects at one time, therefore it isn't being read and wrote at the same time. However, this could be a problem in the future. – Gandalf1783 Jan 29 '22 at 18:05
  • @Gandalf1783 You mean regarding the virtual destructor? At some point you are going to destroy the `ProgressBar` object, but since the only pointer you have to it is a `UiObject` pointer in the vector, I assume that you are going to call `delete` on that pointer. But if `delete` is called through a base of the object, then that base class must have a virtual destructor, otherwise the program has undefined behavior as per the standard. Essentially the virtual destructor is required to tell `delete` that it must look for the derived object to properly destruct it. – user17732522 Jan 29 '22 at 18:12
  • `std::unique_ptr` would also call `delete` on the `UiObject*` pointer when it is destroyed. The mutex is required even if you add only one object, except if you start the thread only after adding the object. – user17732522 Jan 29 '22 at 18:12
  • @Gandalf1783 Also, don't use `new`/`delete` manually. Look up how to use `std::unique_ptr` and use it as I mentioned. Using `new`/`delete` directly will only cause you more trouble in the future. – user17732522 Jan 29 '22 at 18:14
0

I add the UiObjects to the UiManager using this:

It looks like you're allocating these ProgressBar objects on the stack. I don't have all of your code, but I wonder if this is being done in another function and then the stack doesn't exist when they're being used. This would explain the intermittent error as sometimes the stack for that function call still exists. To fix this, I'd allocate the ProgressBar objects on the heap:

 ProgressBar* pBar = new ProgressBar();
 pBar->maxProgress = 60*15;
 pBar->progress = 1;

 uiArray->push_back(pBar);
Grifball
  • 756
  • 1
  • 5
  • 14
  • No, I only allocate them on the stack. I didn't knew that I had to take care of this, I am used to Java where all of this kind of happens behind the scenes. Thanks for the info! – Gandalf1783 Jan 29 '22 at 18:01