https://bugs.kde.org/show_bug.cgi?id=514763
Bug ID: 514763
Summary: Kdenlive Code Audit Report
Classification: Applications
Product: kdenlive
Version First git-master
Reported In:
Platform: Other
OS: Linux
Status: REPORTED
Severity: normal
Priority: NOR
Component: Documentation
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---
Date: 2026-05-21 Target: Kdenlive Master Branch Scope: Deep pattern analysis
for memory safety, command injection, and resource exhaustion.
Summary
A comprehensive audit of the Kdenlive has identified 2 critical/high severity
vulnerabilities and several code quality improvements.
The most significant finding is a Heap Buffer Overflow in mltdevicecapture.cpp
triggered by an integer overflow during image size calculation. Additionally, a
Denial of Service (DoS) vector was confirmed in the audio levels generation
logic where excessive memory can be allocated based on media inputs.
1. Critical Vulnerabilities
1.1. Heap Buffer Overflow in
mltdevicecapture.cpp
Severity: Critical Location:
src/capture/mltdevicecapture.cpp lines 200, 211, 296 Vulnerability Type:
Integer Overflow leading to Heap Buffer Overflow
Description: The code calculates the size for a memcpy operation using the
expression width * height * 3.
memcpy(qimage.bits(), image, size_t(width * height * 3));
The variables width and height are
int types (derived from MLT). If specific large values are supplied (e.g.,
width = 40000, height = 40000), the multiplication width * height * 3 can
overflow a 32-bit signed integer, wrapping around to a small positive number or
negative number (cast to a large size_t). However, the QImage buffer
qimage.bits() is allocated based on the correct or internal properties, but if
the source
image buffer from MLT is larger than the calculated wrapped size, or if the
calculation itself mismatches the actual QImage byte count expectations, a
crash or memory corruption occurs. Crucially, if the size_t cast happens after
the overflow, a very small size might be passed to memcpy, potentially hiding
the issue, logic-wise. But slightly different values could cause a heap
overflow if the destination buffer is smaller than the source data being copied
due to mismatched size logic.
Recommendation: Use size_t (or qint64) for the multiplication to prevent
overflow, and validate dimensions against reasonable maximums before
allocation.
size_t dataSize = static_cast<size_t>(width) * height * 3;
// Ensure dataSize matches destination capacity
if (dataSize > static_cast<size_t>(qimage.sizeInBytes())) return;
memcpy(qimage.bits(), image, dataSize);
1.2. Denial of Service (Memory Exhaustion) in Audio Generators
Severity: High Location:
src/jobs/audiolevels/generators.cpp Vulnerability Type: Unbounded Memory
Allocation
Description: The application allocates a QVector<int16_t> levels whose size is
determined by:
lengthInFrames * AUDIOLEVELS_POINTS_PER_FRAME * channels
lengthInFrames: Sourced from media duration (can be arbitrarily large).
channels: Sourced from media metadata (can be manipulated).
A malicious media file claiming to have 100 channels and an extremely long
duration could force Kdenlive to attempt a multi-gigabyte allocation, causing
the application to crash (OOM) or system instability.
Recommendation: Implement a hard limit on the maximum buffer size. Process
audio levels in chunks or stream them rather than loading the entire analysis
into a monolithic vector.
2. Security Findings
2.1. Path Traversal Risks
Severity: Low (Mitigated) Analysis: Multiple QFile::open(..., WriteOnly) calls
were investigated.
RenderRequest.cpp: The application constructs filenames using user-supplied
guide names. However, correct sanitization replace('/', '_') was found in lines
482-485, preventing directory traversal.
ArchiveWidget.cpp: Construct paths for project extraction. Reliance on
standard KZip/KTar libraries and QDir logic appears standard, though precaution
is advised when extracting untrusted archives.
2.2. Command Injection
Severity: Low Analysis:
QProcess Usage: Extensive use of QProcess for ffmpeg and Python scripts.
Mitigation: Arguments are generally passed as a list (QStringList),
preventing shell injection.
Risk: RecManager.cpp uses KdenliveSettings::grab_parameters(). If a user is
tricked into importing a malicious config file with shell metacharacters in
these parameters, it could be a risk, but this requires significant prior
compromise (configuration injection).
3. Code Quality & Stability
3.1. Infinite Loops
Status: Safe Identified while(true) loops in
TimelineController and ManageSubtitles were manually verified to have correct
break conditions tied to user interaction, preventing infinite execution.
3.2. Raw Pointers
Status: Acceptable Legacy The codebase uses raw pointers in some legacy areas
(AudioCorrelation), but modern components correctly use std::unique_ptr and
Qt's parent-child ownership model.
Module: Video Rendering (src/render, src/dialogs/renderwidget.cpp) Focus: Bugs,
Performance, Security, and Code Quality
Critical Findings
1. Memory Leak in RenderWidget::slotPrepareExport2
Type: Memory Leak Location: src/dialogs/renderwidget.cpp lines 965-1058
Description: A RenderRequest object is allocated on the heap:
RenderRequest *request = new RenderRequest();
The function uses it to generate jobs and check for errors, but never deletes
it. The request object simply leaks every time the user attempts to render.
Recommendation: Use std::unique_ptr<RenderRequest> request =
std::make_unique<RenderRequest>(); or explicitly delete request; at the end of
the function.
2. UI Freezing (Blocking I/O on Main Thread)
Type: Responsiveness / Performance Location: src/dialogs/renderwidget.cpp line
1027 calls request->process() Description: RenderRequest::process() performs
heavy I/O operations:
Synchronous XML parsing (doc.setContent).
Massive file writing (QFile::copy, Xml::docContentToFile).
Directory creation. Since slotPrepareExport2 is a Qt slot connected to a UI
button, this entire process runs on the Main UI Thread, causing Kdenlive to
freeze (become unresponsive) during the "Preparing" phase of large projects.
Recommendation: Move RenderRequest::process() to a worker thread (e.g.,
QtConcurrent::run).
3. Temporary File Leak
Type: Resource Leak Location: src/render/renderrequest.cpp line 339
Description:
QTemporaryFile tmp(...);
tmp.setAutoRemove(false);
The code explicitly disables auto-removal of temporary MLT playlist files so
they can be passed to the render process. However, there is no clear mechanism
in RenderWidget or RenderJobItem to clean up these files after the render job
completes (successfully or failed). Over time, /tmp will fill with .mlt files.
Recommendation: Track these files in RenderJobItem and delete them in the
destructor or upon job completion/abort.
4. Race Condition in Job Status
Type: Concurrency Bug Location: src/dialogs/renderwidget.cpp (Method
checkRenderStatus) Description: checkRenderStatus assumes a job has started if
it sets the status to STARTINGJOB. However, startRendering (line 1179) might
fail proc.startDetached(), setting the status back to FAILEDJOB. If
checkRenderStatus runs concurrently or the status logic is slightly out of sync
(e.g. m_renderStatus update timing), the UI might get stuck in a "Rendering"
state when no process is actually running.
5. Inefficient XML Deep Copying in Loops
Type: Performance Location: src/render/renderrequest.cpp lines 231, 253, 291
Description: Inside loops iterating over render sections or passes:
QDomDocument sectionDoc = doc.cloneNode(true).toDocument();
This performs a deep copy of the entire DOM tree (project file). For complex
projects with thousands of clips, cloning the DOM repeatedly is extremely
expensive and memory-intensive. Recommendation: Optimize by using shared data
structures or modifying the DOM in place if possible (though difficult with
XML). At minimum, audit if the clone is strictly necessary for every iteration.
Architectural & Quality Issues
6. Architectural Violation: UI Logic in Backend Class
Type: Code Smell Location: src/render/renderrequest.cpp lines 372, 384
Description: RenderRequest is a data/logic processing class in src/render.
However, it explicitly calls:
QInputDialog::getText
KMessageBox::questionTwoActions This creates a hard dependency on QtWidgets and
makes unit testing impossible without visual interruption. It also breaks if
the code is moved to a background thread (see finding #2 (closed)).
Recommendation: Pass a "UserInteraction" interface or callback to RenderRequest
to handle prompts, or resolve these questions in RenderWidget before creating
the request.
7. RenderServer Security / Spoofing
Type: Security Location: src/render/renderserver.cpp Description: RenderServer
listens on a local socket based on PID. Any local process can connect and send
JSON commands. There is no authentication. A malicious local process could send
fake setRenderingFinished signals to confuse the user or abort jobs by spoofing
the protocol.
8. Unchecked XML Parsing Errors
Type: Stability Location: src/render/renderrequest.cpp line 155 Description:
The code calls doc.setContent(&file) (line 158) and returns {} on failure.
However, it doesn't propagate the exact parsing error to the user, only a
generic failure. This makes debugging corrupt project files during export
difficult.
9. Hardcoded Special Case strings
Type: Maintainability Location: src/render/renderrequest.cpp (General)
Description: Heavy reliance on magic strings like "kdenlive:uuid",
"kdenlive:audio_track", "kdenlive:track_name". Recommendation: Move these to a
Definitions or KdenliveNamespace constant file to ensure compile-time safety.
10. Inefficient "Todo" Logic
Type: Efficiency Location: src/render/renderrequest.cpp line 248 Description: A
TODO comment explicitly admits to performing an unnecessary file copy: //
QFile::copy(job.playlistPath, newJob.playlistPath); // TODO: if we have a
single item in sections this is unnesessary and produces just unused files This
confirms technical debt that wastes disk I/O.
11. Unsafe Environment Variable Modification
Type: Thread Safety Location: src/main.cpp line 224 Description:
qputenv("MLT_REPOSITORY_DENY", ...) modifies global environment variables.
While currently in main, if this pattern persists or moves (e.g.,
enabling/disabling hardware accel at runtime), it is thread-unsafe in C++.
12. Deprecated Qt Attributes
Type: Maintenance Location: src/main.cpp line 220 Description:
QCoreApplication::setAttribute(Qt::AA_UseDesktopOpenGL, true); is deprecated in
Qt 6 and may vary in behavior across platforms in Qt 5.15+.
13. Hardcoded OS Paths
Type: Portability Location: src/render/renderrequest.cpp lines 309-311
Description: Manual selection of NUL vs /dev/null using #ifdef. Qt provides
QIODevice or QFile mechanisms that might abstract this, or use
QProcess::setStandardOutputFile(QProcess::Null) instead of passing a path
string to an external program if possible.
14. Magic Number in Buffer Calculation (Revisited)
Type: Stability Location: src/jobs/audiolevels/generators.cpp Description: The
audit confirmed usage of AUDIOLEVELS_POINTS_PER_FRAME (5) mixed with
user-controlled variables. While a DoS risk, it's also a "Coding Issue" where a
hardcoded constant drives memory allocation strategy without bounds checking.
15. Lack of "Safe" Overwrite
Type: UX / Data Safety Location: src/dialogs/renderwidget.cpp Description: When
checking for existing files, the code asks the user. If they say "Overwrite",
it proceeds. However, if the render fails halfway, the original file is already
destroyed (truncated). Recommendation: Render to a temporary filename (e.g.,
video.mp4.part) and rename only on success.
--
You are receiving this mail because:
You are watching all bug changes.
