From a4dfc62f0fdd4381912c5145f5f33f55b2e9a896 Mon Sep 17 00:00:00 2001 From: Mike Solar Date: Sat, 1 Aug 2026 19:14:31 +0800 Subject: [PATCH] fix: memory-safety and playback regressions found via ASan - nodeparamview: add visited set to get_distance_between_nodes, fixing unbounded recursion (stack overflow) when the node graph has a cycle - viewerdisplay: give texture_ a consistent owner via assign_texture(); borrowed queue textures are now retained, created ones freed, fixing a dangling pointer that corrupted the heap and crashed in the GL driver - playbackcache: resignal_requests() iterates a copy, handlers may clear_request_range() while iterating (ASan container-overflow) - preview C API: preview request ticket lambdas captured the request state raw; after oakengine_preview_request_free the ticket outlived the request and the finished callback wrote into freed memory (heap-use-after-free). The finished flag is now a shared_ptr captured weakly by the callbacks - playback: oak_playback_frame regains a timestamp (num/den) filled from olive::Frame; the viewer queue append no longer uses Rational() for every frame, which made append_timewise drop all but the first frame and froze the picture during playback - mainwindow: open_node_in_viewer refuses sequence nodes; sequences already have the Sequence Viewer, and saved layouts could otherwise resurrect a redundant floating Viewer bound to the sequence --- app/widget/nodeparamview/nodeparamview.cpp | 17 ++++++- app/widget/viewer/viewer.cpp | 4 +- app/widget/viewer/viewerdisplay.cpp | 42 +++++++++++------ app/widget/viewer/viewerdisplay.h | 20 +++++++++ app/window/mainwindow/mainwindow.cpp | 8 ++++ engine/include/oakengine/preview.h | 2 + engine/render/playbackcache.h | 5 ++- engine/src/capi/preview.cpp | 52 +++++++++++++++++----- 8 files changed, 120 insertions(+), 30 deletions(-) diff --git a/app/widget/nodeparamview/nodeparamview.cpp b/app/widget/nodeparamview/nodeparamview.cpp index f55a07eab..eb317308a 100644 --- a/app/widget/nodeparamview/nodeparamview.cpp +++ b/app/widget/nodeparamview/nodeparamview.cpp @@ -951,18 +951,25 @@ void NodeParamView::add_node(OakEngineNode *n, OakEngineNode *ctx, NodeParamView } } -int get_distance_between_nodes(OakEngineNode *start, OakEngineNode *end) +int get_distance_between_nodes(OakEngineNode *start, OakEngineNode *end, + QSet *visited) { if (start == end) { return 0; } + if (visited->contains(start)) { + return -1; + } + visited->insert(start); + const oak::Node start_node(start); const int connection_count = start_node.input_connection_count_all(); for (int i = 0; i < connection_count; i++) { OakEngineNode *upstream = start_node.input_connection_at_all(i).node.handle(); - int this_node_dist = get_distance_between_nodes(upstream, end); + int this_node_dist = + get_distance_between_nodes(upstream, end, visited); if (this_node_dist != -1) { return 1 + this_node_dist; } @@ -971,6 +978,12 @@ int get_distance_between_nodes(OakEngineNode *start, OakEngineNode *end) return -1; } +int get_distance_between_nodes(OakEngineNode *start, OakEngineNode *end) +{ + QSet visited; + return get_distance_between_nodes(start, end, &visited); +} + void NodeParamView::sort_items_in_context(NodeParamViewContext *context_item) { QVector> distances; diff --git a/app/widget/viewer/viewer.cpp b/app/widget/viewer/viewer.cpp index b3bb26d05..00b0871a2 100644 --- a/app/widget/viewer/viewer.cpp +++ b/app/widget/viewer/viewer.cpp @@ -1836,7 +1836,9 @@ void ViewerWidget::renderer_generated_frame_for_queue() QVariant frame = QVariant::fromValue(f); foreach (ViewerDisplayWidget *dw, playback_devices_) { - dw->queue()->append_timewise({ Rational(), frame }, + dw->queue()->append_timewise({ Rational(pf.timestamp_num, + pf.timestamp_den), + frame }, playback_speed_); } } diff --git a/app/widget/viewer/viewerdisplay.cpp b/app/widget/viewer/viewerdisplay.cpp index a8743d763..b8cb5775f 100644 --- a/app/widget/viewer/viewerdisplay.cpp +++ b/app/widget/viewer/viewerdisplay.cpp @@ -108,6 +108,18 @@ ViewerDisplayWidget::~ViewerDisplayWidget() MANAGEDDISPLAYWIDGET_DEFAULT_DESTRUCTOR_INNER; } +void ViewerDisplayWidget::assign_texture(void *t, bool owned) +{ + if (owned_texture_) { + oakengine_display_texture_free(owned_texture_); + } + if (t && !owned) { + oakengine_display_texture_retain(t); + } + owned_texture_ = t; + texture_ = t; +} + void ViewerDisplayWidget::set_matrix_translate(const QMatrix4x4 &mat) { translate_matrix_ = mat; @@ -483,10 +495,11 @@ void ViewerDisplayWidget::on_paint() oakengine_display_texture_height(texture_) != frame_vp.height || oakengine_display_texture_format(texture_) != frame_vp.format || oakengine_display_texture_channel_count(texture_) != 4)) { - texture_ = oakengine_display_texture_create( - renderer(), &frame_vp, - oakengine_codec_frame_data(load_handle), - oakengine_codec_frame_linesize(load_handle)); + assign_texture(oakengine_display_texture_create( + renderer(), &frame_vp, + oakengine_codec_frame_data(load_handle), + oakengine_codec_frame_linesize(load_handle)), + true); } else if (!drew_backend_neutral_frame) { oakengine_display_texture_upload( texture_, oakengine_codec_frame_data(load_handle), @@ -499,7 +512,7 @@ void ViewerDisplayWidget::on_paint() src_ren && src_ren != renderer()) { if (oakengine_display_renderer_is_open_gl(src_ren) && oakengine_display_renderer_is_open_gl(renderer())) { - texture_ = load_handle; + assign_texture(load_handle, false); } else { // Cross-backend: download and re-upload void *tmp_frame = oakengine_codec_frame_create(); @@ -513,24 +526,25 @@ void ViewerDisplayWidget::on_paint() &tex_params, oakengine_codec_frame_data(tmp_frame), oakengine_codec_frame_linesize(tmp_frame)); - texture_ = oakengine_display_texture_create( - renderer(), &tex_params, - oakengine_codec_frame_data(tmp_frame), - oakengine_codec_frame_linesize(tmp_frame)); + assign_texture(oakengine_display_texture_create( + renderer(), &tex_params, + oakengine_codec_frame_data(tmp_frame), + oakengine_codec_frame_linesize(tmp_frame)), + true); } else { - texture_ = load_handle; + assign_texture(load_handle, false); } oakengine_codec_frame_free(tmp_frame); } } else if (!drew_backend_neutral_frame) { - texture_ = load_handle; + assign_texture(load_handle, false); } } else { - texture_ = load_custom_texture_from_frame(load_frame_); + assign_texture(load_custom_texture_from_frame(load_frame_), true); } if (drew_backend_neutral_frame) { - texture_ = nullptr; + assign_texture(nullptr, true); } emit texture_changed(texture_); @@ -762,7 +776,7 @@ void ViewerDisplayWidget::on_destroy() super::on_destroy(); - texture_ = nullptr; + assign_texture(nullptr, true); if (deinterlace_texture_) { oakengine_display_texture_free(deinterlace_texture_); deinterlace_texture_ = nullptr; diff --git a/app/widget/viewer/viewerdisplay.h b/app/widget/viewer/viewerdisplay.h index c0ca0fdd9..fde45965e 100644 --- a/app/widget/viewer/viewerdisplay.h +++ b/app/widget/viewer/viewerdisplay.h @@ -338,9 +338,29 @@ private: /** * @brief Internal reference to the OpenGL texture to draw. Set in SetTexture() and used in paintGL(). + * + * Never assign directly; use assign_texture() so ownership stays consistent. */ void *texture_; + /** + * @brief Owned (retained or created) reference backing texture_. + * + * texture_ may point to a texture owned by the playback queue's + * OakSharedBuffer; this member holds our own reference so texture_ can + * never dangle after the queue entry is replaced. + */ + void *owned_texture_ = nullptr; + + /** + * @brief Assign texture_ with consistent ownership. + * + * Releases the previous owned reference. If owned is true, takes over the + * passed reference (e.g. freshly created); otherwise retains it. texture_ + * is set to the same pointer (possibly nullptr). + */ + void assign_texture(void *t, bool owned); + /** * @brief Internal texture to deinterlace to */ diff --git a/app/window/mainwindow/mainwindow.cpp b/app/window/mainwindow/mainwindow.cpp index 5628ea9f3..04a930e56 100644 --- a/app/window/mainwindow/mainwindow.cpp +++ b/app/window/mainwindow/mainwindow.cpp @@ -318,6 +318,14 @@ void MainWindow::open_folder(OakEngineNode *i, bool floating) void MainWindow::open_node_in_viewer(OakEngineNode *node) { + // Sequences already have the dedicated Sequence Viewer. Opening a + // floating Viewer on a Sequence just duplicates it and looks like the + // two viewers' controls are swapped. This also filters out stale + // entries pointing at sequences in saved project layouts. + if (!node || oak::Node(node).is_sequence()) { + return; + } + ViewerPanel *existing = nullptr; for (auto it = viewer_panels_.cbegin(); it != viewer_panels_.cend(); it++) { diff --git a/engine/include/oakengine/preview.h b/engine/include/oakengine/preview.h index 237e56fa9..b77d44623 100644 --- a/engine/include/oakengine/preview.h +++ b/engine/include/oakengine/preview.h @@ -78,6 +78,8 @@ typedef struct oak_playback_frame { int format; /**< olive::PixelFormat::Format value. */ const void *data; /**< Planar data pointer (first plane). */ int linesize; /**< Bytes per row of the first plane. */ + int64_t timestamp_num; /**< Frame timestamp (seconds) as a rational. */ + int64_t timestamp_den; /**< 0 when the frame carries no timestamp. */ } oak_playback_frame; /** diff --git a/engine/render/playbackcache.h b/engine/render/playbackcache.h index a6cc8db96..408ae624d 100644 --- a/engine/render/playbackcache.h +++ b/engine/render/playbackcache.h @@ -131,7 +131,10 @@ public: void resignal_requests() { - for (const TimeRange &r : requested_) { + // Iterate over a copy: handlers may call clear_request_range() and + // mutate requested_ while we're iterating it. + const TimeRangeList requests = requested_; + for (const TimeRange &r : requests) { emit requested(request_context_, r); } } diff --git a/engine/src/capi/preview.cpp b/engine/src/capi/preview.cpp index 6778803e2..7555e40e8 100644 --- a/engine/src/capi/preview.cpp +++ b/engine/src/capi/preview.cpp @@ -429,7 +429,12 @@ int oakengine_preview_cacher_force_cache_range(OakEngineNode *node, struct OakEnginePreviewRequestState { olive::RenderTicketPtr ticket; - std::atomic finished{ false }; + // Shared finished flag. Ticket completion lambdas capture a weak_ptr to + // this flag so that freeing the request (oakengine_preview_request_free) + // detaches them instead of leaving dangling writes/callbacks behind — + // the ticket can outlive the request inside the RenderManager. + std::shared_ptr> finished = + std::make_shared>(false); bool has_frame = false; bool has_audio = false; // Video result @@ -460,8 +465,13 @@ oakengine_preview_request_single_frame(OakEngineNode *viewer, int64_t num, s->ticket = olive::RenderManager::instance()->get_cacher()->get_single_frame( v, olive::Rational(num, den), dry != 0); if (s->ticket) { - QObject::connect(s->ticket.get(), &olive::RenderTicket::finished, - [s]() { s->finished.store(true); }); + QObject::connect( + s->ticket.get(), &olive::RenderTicket::finished, + [weak = std::weak_ptr>(s->finished)] { + if (auto f = weak.lock()) { + f->store(true); + } + }); } return reinterpret_cast(s); } @@ -487,8 +497,13 @@ oakengine_preview_request_audio_range(OakEngineNode *viewer, int64_t in_num, v, olive::TimeRange(olive::Rational(in_num, in_den), olive::Rational(out_num, out_den))); if (s->ticket) { - QObject::connect(s->ticket.get(), &olive::RenderTicket::finished, - [s]() { s->finished.store(true); }); + QObject::connect( + s->ticket.get(), &olive::RenderTicket::finished, + [weak = std::weak_ptr>(s->finished)] { + if (auto f = weak.lock()) { + f->store(true); + } + }); } return reinterpret_cast(s); } @@ -500,7 +515,7 @@ int oakengine_preview_request_is_done(const OakEnginePreviewRequest *req) } const OakEnginePreviewRequestState *s = reinterpret_cast(req); - return s->ticket && s->finished.load() ? 1 : 0; + return s->ticket && s->finished->load() ? 1 : 0; } int oakengine_preview_request_has_result(const OakEnginePreviewRequest *req) @@ -524,10 +539,18 @@ int oakengine_preview_request_set_finished_callback( if (!s->ticket) { return OAKENGINE_E_INVALID; } - // Connect the ticket's finished signal to call the callback. + // Connect the ticket's finished signal to call the callback. Guarded by + // the request's shared flag so a freed request suppresses the callback. if (callback) { - QObject::connect(s->ticket.get(), &olive::RenderTicket::finished, - [callback, user_data]() { callback(user_data); }); + QObject::connect( + s->ticket.get(), &olive::RenderTicket::finished, + [callback, user_data, + weak = std::weak_ptr>(s->finished)] { + if (auto f = weak.lock()) { + f->store(true); + callback(user_data); + } + }); } return OAKENGINE_OK; } @@ -544,7 +567,7 @@ int oakengine_preview_request_get_frame(OakEnginePreviewRequest *req, return OAKENGINE_E_INVALID; } // Wait for finish if not done (pump events). - if (!s->finished.load()) { + if (!s->finished->load()) { QCoreApplication::processEvents(); return OAKENGINE_E_INVALID; } @@ -567,6 +590,11 @@ int oakengine_preview_request_get_frame(OakEnginePreviewRequest *req, out->height = s->frame_height; out->format = s->frame_format; out->data = s->frame->data(); + { + const olive::Rational ts = s->frame->timestamp(); + out->timestamp_num = ts.numerator(); + out->timestamp_den = ts.denominator(); + } out->linesize = s->frame->linesize_bytes(); return OAKENGINE_OK; } @@ -579,7 +607,7 @@ int oakengine_preview_request_get_audio_channel_count( } const OakEnginePreviewRequestState *s = reinterpret_cast(req); - if (!s->ticket || !s->ticket->has_result() || !s->finished.load()) { + if (!s->ticket || !s->ticket->has_result() || !s->finished->load()) { return 0; } if (!s->has_audio) { @@ -614,7 +642,7 @@ int oakengine_preview_request_get_audio_samples( } OakEnginePreviewRequestState *s = reinterpret_cast(req); - if (!s->ticket || !s->ticket->has_result() || !s->finished.load()) { + if (!s->ticket || !s->ticket->has_result() || !s->finished->load()) { return OAKENGINE_E_INVALID; } if (!s->has_audio) {