Skip to content

Conversation

@shermp
Copy link
Collaborator

@shermp shermp commented Jun 1, 2021

This PR fixes #107

The previous code simply passed nullptr to the MoreController::dropbox method, instead of a pointer to a valid 'this' pointer. The new code creates a valid MoreController object to pass to the dropbox method.

Tested on my Forma with fw 4.26.16704.

@NiLuJe can you please double check it works on your Forma?

Has anyone got fw 4.23.15505 they could test with? It should work, but probably wouldn't hurt to test...

It may not have been necessary as of FW 15505, but required in later versions
@shermp shermp requested a review from pgaskin as a code owner June 1, 2021 21:03
@NiLuJe
Copy link
Collaborator

NiLuJe commented Jun 1, 2021

Yup, worksforme over here (on 4.26) :).

@shermp
Copy link
Collaborator Author

shermp commented Jun 1, 2021

Note, I'm not entirely sure if this is going to leak memory or not, or if it even matters.

@pgaskin
Copy link
Owner

pgaskin commented Jun 2, 2021

not entirely sure if this is going to leak memory or not

In that case, I usually look at the callers to determine what they do. Note that even though in this situation, no functions appear to reference the PLT entry (which calls the main symbol entry) directly, there is a reference to the "bx pc" instruction above it. This is used to switch between ARM/Thumb when tail-calling the function.

Currently, the only reference detected is a switch table, which is referenced by the Qt metacall. Thus, it's probably used in another widget by a signal connection.

Now, we look for references to the MoreController constructor, and can figure out how it's used by inspecting its callers. In short, this leads us to the MainWindowController, which we can see deletes views after they are popped. This isn't directly relevant to us, but it tells us that any views handled by the MainWindowController must be eap-allocated. This is confirmed the other reference to the MoreController constructor (the Activity view's back button).

Next, we inspect the MoreController's construction itself to see if it allocates anything which could come back to bite us later (by causing memory leaks or referencing itself later). From that, it's pretty clear that it's just a plain QObject (which doesn't need to be properly destructed, we can do what we currently do with it on the stack) and some *NavMixins allocated on the MoreController's memory, which do not have anything relevant in their constructor.

Finally, just to make sure there aren't any other references outside the function call (we don't expect it since MWC is usually in charge of the MC's lifetime, and it can pretty much delete it whenever it wants), we can check the Dropbox function. It is as we expect.

Based on this, it's now clear that at least in this version, it's safe to either stack-allocate the memory used for the MoreController, or heap-allocate it and tell the QObject to delete itself later or call D0. In this context, both are just as likely to break in the future if Kobo decides to start using the MoreController later, so I'd go for the first one because it's simpler. If they did, the problem would show up as a crash when the memory is attempted to be freed later either stack corruption. Because of the location and the purpose, it should fail relatively soon after in this situation, and it should be relatively easy to diagnose. If it wasn't, I'd use the heap-allocation option to make it easier to track down.

To answer your question directly, it won't leak memory directly since it doesn't allocate any within the object, and you're stack-allocating the object itself (edit: I misread the diff, you used calloc, not alloca).

Copy link
Owner

@pgaskin pgaskin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but it'd be nice if someone could test this on an older version to be sure (I'll do it if nobody else does). This would just be to ensure it doesn't cause a regression (it doesn't need to support Dropbox -- it'll show a blank screen in this case, but it shouldn't cause a crash).

@shermp
Copy link
Collaborator Author

shermp commented Jun 2, 2021

Actually, I've heap allocated with calloc, can switch to alloca if you prefer.

@pgaskin
Copy link
Owner

pgaskin commented Jun 2, 2021

Actually, I've heap allocated with calloc, can switch to alloca if you prefer.

Oops, appears I misread that while doing this from my phone. In that case, it does indeed leak memory, but you should be safe just deleting it. But now that I think about it, we might as well do this properly. Call _ZN14MoreControllerD0 on the pointer after you launch dropbox.

@shermp
Copy link
Collaborator Author

shermp commented Jun 2, 2021

Oops, appeared I misread that while doing this from my phone. In that case, it does indeed leak memory, but you should be safe just deleting it. But now that I think about it, we might as well do this properly. Call _ZN14MoreControllerD0 on the pointer after you launch dropbox.

Yeah, was thinking that calling the destructor was probably the right thing to do, just wasn't sure if the MoreController would end up belonging to another QObject somewhere along the way and be deleted elsewhere.

I'm a bit hazy on C++ destructors. I THINK I will need to call free after calling the destructor. is this correct? (Obviously C++ new and delete do this automatically here)

@pgaskin
Copy link
Owner

pgaskin commented Jun 2, 2021

I'm a bit hazy on C++ destructors.

The C++ ABI document is your friend. D0 will delete it for you, and D2 will let you delete it yourself. Note that D2 is essentially a no-op right now (which is why it's currently safe to stack allocate it), and D0 is just D2+delete. Also, it's up to the compiler to decide which ones it needs to generate, so not all objects will have a D0.

So, no, you don't need to free it if calling D0. Also, technically, it's undefined behavior for it to delete malloc'd memory (one is C, the other is C++), but they should be identical in practice. To be safe, you could allocate it with operator new instead if you want.

To avoid UB, use ::operator new() to allocate memory
@shermp
Copy link
Collaborator Author

shermp commented Jun 2, 2021

Alright, hopefully no more leaky memory.

@pgaskin pgaskin merged commit 649bfa6 into master Jun 3, 2021
@pgaskin pgaskin deleted the shermp/dropbox-fix branch June 3, 2021 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nickel_open:library:dropbox crashing on 4.26.16704

4 participants