Fix multiple crashes related to incorrect rz_iterator usage.

RzAnalysisBytes are owned by the iterator, so returning analysis bytes while destroying iterator owning it is incorrect.
rizinorg · Mar 3, 2024 · efa29b6 · efa29b6
1 parent 762830d
commit efa29b6
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 16 deletions.
diff --git a/src/core/Cutter.cpp b/src/core/Cutter.cpp
@@ -715,18 +715,16 @@ void CutterCore::delFlag(const QString &name)
     emit flagsChanged();
 }
 
-PRzAnalysisBytes CutterCore::getRzAnalysisBytesSingle(RVA addr)
+CutterRzIter<RzAnalysisBytes> CutterCore::getRzAnalysisBytesSingle(RVA addr)
 {
     CORE_LOCK();
     ut8 buf[128];
     rz_io_read_at(core->io, addr, buf, sizeof(buf));
 
-    auto seek = seekTemp(addr);
-    auto abiter = fromOwned(rz_core_analysis_bytes(core, addr, buf, sizeof(buf), 1));
-    auto ab =
-            abiter ? reinterpret_cast<RzAnalysisBytes *>(rz_iterator_next(abiter.get())) : nullptr;
-
-    return { ab, rz_analysis_bytes_free };
+    // Warning! only safe to use with stack buffer, due to instruction count being 1
+    auto result =
+            CutterRzIter<RzAnalysisBytes>(rz_core_analysis_bytes(core, addr, buf, sizeof(buf), 1));
+    return result;
 }
 
 QString CutterCore::getInstructionBytes(RVA addr)

diff --git a/src/core/Cutter.h b/src/core/Cutter.h
@@ -62,8 +62,6 @@ struct CUTTER_EXPORT RegisterRef
     QString name;
 };
 
-using PRzAnalysisBytes = std::unique_ptr<RzAnalysisBytes, decltype(rz_analysis_bytes_free) *>;
-
 class CUTTER_EXPORT CutterCore : public QObject
 {
     Q_OBJECT
@@ -248,7 +246,7 @@ class CUTTER_EXPORT CutterCore : public QObject
     QString getGlobalVariableType(RVA offset);
 
     /* Edition functions */
-    PRzAnalysisBytes getRzAnalysisBytesSingle(RVA addr);
+    CutterRzIter<RzAnalysisBytes> getRzAnalysisBytesSingle(RVA addr);
     QString getInstructionBytes(RVA addr);
     QString getInstructionOpcode(RVA addr);
     void editInstruction(RVA addr, const QString &inst, bool fillWithNops = false);

diff --git a/src/core/RizinCpp.h b/src/core/RizinCpp.h
@@ -51,12 +51,6 @@ static inline auto fromOwned(RZ_OWN RzList *data) -> UniquePtrCP<decltype(data),
     return { data, {} };
 }
 
-static inline auto fromOwned(RZ_OWN RzIterator *data)
-        -> UniquePtrCP<decltype(data), &rz_iterator_free>
-{
-    return { data, {} };
-}
-
 // Rizin list iteration macros
 // deprecated, prefer using CutterPVector and CutterRzList instead
 #define CutterRzListForeach(list, it, type, x)                                                     \
@@ -170,4 +164,30 @@ class CutterRzList
     iterator end() const { return iterator(nullptr); }
 };
 
+template<typename T>
+class CutterRzIter
+{
+    UniquePtrC<RzIterator, &rz_iterator_free> rzIter;
+
+public:
+    CutterRzIter(RzIterator *rzIter) : rzIter(rzIter)
+    {
+        // immediately attempt advancing by 1, otherwise it's hard to distinguish whether current
+        // element is null due to not having called next, or due to having run out of elements
+        if (rzIter) {
+            ++*this;
+        }
+    }
+
+    CutterRzIter<T> &operator++()
+    {
+        rz_iterator_next(rzIter.get());
+        return *this;
+    }
+    operator bool() { return rzIter && rzIter->cur; }
+    T &operator*() { return *reinterpret_cast<RzAnalysisBytes *>(rzIter->cur); }
+    T *get() { return reinterpret_cast<RzAnalysisBytes *>(rzIter->cur); }
+    T *operator->() { return reinterpret_cast<RzAnalysisBytes *>(rzIter->cur); }
+};
+
 #endif // RIZINCPP_H