Segfault / undefined behavior in Timeline::Close() #378
Comments
|
Oops, no, that erase call shouldn't be the issue (it's not called on the currently iterated over vector but on a map). |
|
Yeah, I was gonna say, it's modifying That traceback is still pretty ugly, though. Wonder what is going on there? |
|
I don't get why And why would it be its responsibility to do things like this?: Lines 81 to 90 in 18811f4 If the None of the above is actually relevant, though....Anyway, clearly in your trace it's not even getting that far. I think the traceback is a simple concurrency issue, there's waaaaaaaayy too much code that executes between this: Lines 77 to 79 in 18811f4 and this: Lines 705 to 706 in 18811f4 At the very least, I think it should look something more like this: Timeline::~Timeline() {
if (is_open) {
// Auto Close if not already
is_open = false;
Close();
}...To reduce the chances that two threads will both try to close the same Timeline. (If not actual locking to ensure it doesn't happen.) |
|
Also, Lines 654 to 663 in 18811f4 Clearly, the Timeline destructor should not be doing any of that. |
|
The problem with That's all well and good, but it means that calling Instead of calling The whole reason for this: Lines 85 to 88 in 18811f4 was apparently a fix to prevent previous destructor crashes, because FrameMapper::~FrameMapper() does call its own Close() — and there again, Close() is doing lots of complicated things if reader is non-null:libopenshot/src/FrameMapper.cpp Lines 644 to 674 in 18811f4 ...But am I wrong in thinking that, rather than |
|
Hm ... indeed, all that But your guess regarding a concurrency issue ... I don't think that's the problem here. I was running in gdb and set a breakpoint at This also rules out my suspected "double" destructor call. What seems to be happening - looking at the assembly - is a call to a computed address. Only that this address is far off from where it should be? I've attached the disassembly of the full function below and marked the line where the segfault happens (with Perhaps this is the |
|
It's that Openshot somewhere apparently calls (through the pyhton interface) Then, when I close Openshot I first get: Which calls the destructor of the object which was passed to So the issue is that Openshot drops the reference to the cache object, Python calls the cache object's destructor, then later calls timelines destructor which expects the cache object to still be valid. libopenshot isn't to blame then, after all (only for that insecure interface ... we should change this) |
|
@musteresel Interesting! Yeah, in looking at the code — this is one of five calls to
# Set MaxSize (so we don't have any downsampling)
self.timeline.SetMaxSize(video_settings.get("width"), video_settings.get("height"))
# Set lossless cache settings (temporarily)
export_cache_object = openshot.CacheMemory(500)
self.timeline.SetCache(export_cache_object)...since Python will keep a reference to (Does kind of feel like libopenshot's fault, though. The Python code never actually does anything destructive or wrong... it just handles an object it's forced to handle due to the design of the current Timeline API.) 😉 Still, in the immediate term it might be enough to just |
|
ARRRRGH! Of course, that won't help us with this code, where OpenShot's Python code not only sees the cache objects, but actually does the work of swapping them! 😬
# Save current cache object and create a new CacheMemory object (ignore quality and scale prefs)
old_cache_object = self.cache_object
new_cache_object = openshot.CacheMemory(settings.get_settings().get("cache-limit-mb") * 1024 * 1024)
self.timeline_sync.timeline.SetCache(new_cache_object)
# Set MaxSize to full project resolution and clear preview cache so we get a full resolution frame
self.timeline_sync.timeline.SetMaxSize(app.project.get("width"), app.project.get("height"))
self.cache_object.Clear()
# Reset the MaxSize to match the preview and reset the preview cache
viewport_rect = self.videoPreview.centeredViewport(self.videoPreview.width(), self.videoPreview.height())
self.timeline_sync.timeline.SetMaxSize(viewport_rect.width(), viewport_rect.height())
self.cache_object.Clear()
self.timeline_sync.timeline.SetCache(old_cache_object)
self.cache_object = old_cache_object
old_cache_object = None
new_cache_object = NoneOr
def InitCacheSettings(self):
"""Set the correct cache settings for the timeline"""
# Load user settings
s = settings.get_settings()
log.info("InitCacheSettings")
log.info("cache-mode: %s" % s.get("cache-mode"))
log.info("cache-limit-mb: %s" % s.get("cache-limit-mb"))
# Get MB limit of cache (and convert to bytes)
cache_limit = s.get("cache-limit-mb") * 1024 * 1024 # Convert MB to Bytes
# Clear old cache
new_cache_object = None
if s.get("cache-mode") == "CacheMemory":
# Create CacheMemory object, and set on timeline
log.info("Creating CacheMemory object with %s byte limit" % cache_limit)
new_cache_object = openshot.CacheMemory(cache_limit)
self.timeline_sync.timeline.SetCache(new_cache_object)
elif s.get("cache-mode") == "CacheDisk":
# Create CacheDisk object, and set on timeline
log.info("Creating CacheDisk object with %s byte limit at %s" % (cache_limit, info.PREVIEW_CACHE_PATH))
image_format = s.get("cache-image-format")
image_quality = s.get("cache-quality")
image_scale = s.get("cache-scale")
new_cache_object = openshot.CacheDisk(info.PREVIEW_CACHE_PATH, image_format, image_quality, image_scale, cache_limit)
self.timeline_sync.timeline.SetCache(new_cache_object)
# Clear old cache before it goes out of scope
if self.cache_object:
self.cache_object.Clear()
# Update cache reference, so it doesn't go out of scope
self.cache_object = new_cache_object |
|
Hm ... passing in a pointer to an externally created cache object (which implements some interface) is IMO a good thing. The issue is we need to handle ownership more clearly, or rather in a way such that it's easier for the python code (and other C++ code) to "do the right thing". We could use a MyCacheObject obj;
some_timeline.SetCache(&obj);
// do workI think the best idea would be to just not use the cache when destructing the Timeline object. It's not really So, i'd say to the user we present this: // If new_cache != NULL, sets new_cache as the cache to use.
// If new_cache == NULL, then Timeline uses no or an internal cache.
//
// Returns a previously set cache or NULL if no cache was set previously. Returns also NULL when the previous cache is an internal one.
CacheBase * Timeline::SetCache(CacheBase * new_cache);So the state of Timeline would be: Timeline tl = /* construction */
// tl now uses an internal or no cache, no deal to the external code
CacheBase * old = tl.SetCache(new MyAwesomeCache());
assert(old == nullptr)
// tl now uses the instance of MyAwesomeCache
old = tl.SetCache(nullptr);
// old is the pointer returned above by new
delete old;
// tl uses an internal or no cache |
|
Basically we don't take ownership of external cache objects, but allow "freeing" the timeline from them. And in |
|
Hrm. That still feels risky to me, but if you think it can work, I'm game. |
|
I've been looking how Timeline uses that cache and ... Eh .... what? Lines 778 to 800 in 18811f4 Line 797 ... it just throws new Frames away? As far as I can see Only later it adds some frame to the cache ... after numerous other stuff it's doing. The big question here is: Who is responsible for working with that cache? Do we want / need a cache per timeline? Per clip? |
Yes ... I just think the ability to use a cache other than the memory backed cache is important. The |
Oh, I don't disagree — and I don't think the ability needs to be interfered with. As you say, Timeline should just not take ownership of external cache references. That's definitely step 1. But I think there's also room for convenience methods that just configure (and/or enable?) that internal cache you mention here:
...So that callers, especially callers in other languages, don't necessarily have to manage it for themselves. (OpenShot's Python code would seem to be an ideal candidate — It doesn't do ANYTHING with the cache other than call one part of the libopenshot API to allocate it, and another part to assign it to the Timeline. Is there any advantage to having the Python code in there playing middleman? If not, my argument would be: Then get it out!) |
Even OpenShot uses In both cases, they'd take the same arguments as the cache objects' constructors, but instead of returning the object to the caller, Timeline would keep track of it, and it would own and manage the allocation (including doing cleanup from the destructor). |
|
The biggest complication to something like what I suggested would be (Though... should it? Maybe there's a cleaner approach to that, anyway.) |
|
While we're on the topic that started this all off originally, I'm trying to grok how the bits of Lines 341 to 358 in 18811f4 libopenshot/src/CacheMemory.cpp Lines 219 to 235 in 18811f4 Documentation seems to conflict / be unclear on whether that's actually OK (or I'm not grasping it fully). The invalidation note for
Buut, exactly that pattern is part of the example code for // Erase all even numbers (C++11 and later)
for (auto it = c.begin(); it != c.end(); ) {
if (*it % 2 == 0) {
it = c.erase(it);
} else {
++it;
}
}I guess, because |
|
Exactly; the returned iterator after these erase calls in the middle is the only valid iterator: auto this_one_is_valid = c.erase(it);
// "it" invalid
it =this_one_is_valid;
// now "it" is valid again |
|
(As an aside, can I just say... while I'm generally of the mindset that braceless conditional blocks are evil, representing a false economy that only leads to later bugs, I can accept that under certain circumstances they make code more concise.* But the idea of pairing a braced IOW: THIS bit of code was born in the pits of hell, style-wise, and I reject it on principle! 😉 libopenshot/src/CacheMemory.cpp Lines 229 to 234 in 18811f4 * – (Though in truth I find that opinion is held mostly by proponents of Allman/GNU bracing, where a block's opening brace adds an entire extra line to the listing. Whereas just using K&R bracing avoids that issue, and if you always use braces, the open brace doesn't need to call attention to itself by being on a separate line.) |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Hands off, bot! |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I'm seeing segmentation faults when I close OpenShot. Today I took the time to look at the stacktrace, and look, libopenshot is to blame:
The culprit is - I think - quickly found in
Timeline::Close():What does
update_open_clips()? I've removed uninteresting comments / logging but added what's happening in this case:eraseinvalides the iterator (clip_itr) ... thus incrementing it is undefined behavior!I'm fixing this as soon as I have time for it, opening this issue so that I don't forget it / it doesn't get lost.
The text was updated successfully, but these errors were encountered: