OpenToonz: من الداخل والخارج

image1.png

لقد مرت أربع سنوات تقريبًا منذ التحقق من PVS-Studio من كود مصدر OpenToonz. هذا المشروع هو أداة قوية للغاية لإنشاء الرسوم المتحركة ثنائية الأبعاد. منذ الفحص الأخير ، بمساعدته ، تم إنشاء أعمال رسوم متحركة مثل Mary و Witch Flower و Batman Ninja و Promare وغيرها. بما أن الاستوديوهات الكبيرة تواصل استخدام Toonz ، فلماذا لا تتحقق من جودة شفرة المصدر مرة أخرى؟

يمكنك التعرف على التحليل السابق للأخطاء في مقالة " شفرة سيئة لحزمة لإنشاء رسوم متحركة ثنائية الأبعاد لـ Toonz ". بشكل عام ، لا يبدو أن جودة الشفرة قد تحسنت كثيرًا ، لذا فإن الانطباع العام ليس أفضل. بالإضافة إلى ذلك ، تم العثور على العديد من الأخطاء نفسها كما في المقالة السابقة. لن نعتبرها مرة أخرى ، لأن هناك العديد من الأشياء للاختيار من بينها.

ومع ذلك ، يجب أن تفهم أن وجود أخطاء لا يمنع بالضرورة الاستخدام النشط والمنتج لمنتج البرنامج. على الأرجح ، الأخطاء التي تم العثور عليها تعيش في أجزاء من التعليمات البرمجية نادرًا ما يتم استخدامها أو غير مستخدمة تمامًا ، وإلا فقد تم تحديدها في عملية استخدام التطبيق وإصلاحها. ومع ذلك ، هذا لا يعني أن التحليل الثابت زائدة عن الحاجة. إنه فقط أن معنى التحليل الثابت ليس البحث عن الأخطاء القديمة وغير ذات الصلة ، ولكن لتقليل تكلفة عملية التطوير. يمكن الكشف عن العديد من الأخطاء عند كتابة التعليمات البرمجية على الفور ، وليس أثناء تشغيل البرنامج. وفقا لذلك ، مع الاستخدام المنتظم لمحلل ثابت ، يتم تصحيح الأخطاء في مرحلة مبكرة. وهذا يوفر وقت المطورين وأموال الشركة ويحسن من إدراك المنتج للمنتج. موافق ، من المزعج إلغاء الاشتراك باستمرار للمطورين ،هذا الشيء أو ذاك لا يعمل.

جزء N1

V610 سلوك غير محدد. تحقق من عامل النقل "<<". المعامل الأيسر "(- 1)" سلبي.

decode_mcu_AC_refine (j_decompress_ptr cinfo, JBLOCKROW *MCU_data)
{
  int p1, m1;
  p1 = 1 << cinfo->Al;    
  m1 = (-1) << cinfo->Al; 
  ....
}

ليس واضحا ما أراد مؤلف الكود تحقيقه هنا. يؤدي استخدام عوامل النقل ذات الأرقام السالبة إلى سلوك غير محدد. تبدو الطريقة التي تم وصف سلوك مشغلي المناوبة في المعيار محيرة بعض الشيء للوهلة الأولى ، لكنها لا تزال تتحقق من ذلك:

1. نوع النتيجة هو المعامل الأيسر الذي تمت ترقيته. السلوك غير محدد إذا كان المعامل الأيمن سالبًا ، أو أكبر من أو يساوي طول وحدات المعامل الأيسر التي تم ترقيتها.

2. قيمة E1 << E2 هي مواضع بت E2 ذات إزاحة يسارية E1 ؛ البتات التي تم إخلاؤها هي صفر مليئة. إذا كان E1 يحتوي على نوع غير موقّع ، فإن قيمة النتيجة هي E1 * 2 ^ E2 ، مع تقليل المعامل واحد أكثر من الحد الأقصى للقيمة التي يمكن تمثيلها في نوع النتيجة. خلاف ذلك ، إذا كان E1 له نوع موقّع وقيمة غير سالبة ، وكان E1 * 2 ^ E2 يمكن تمثيله في نوع النتيجة ، فهذه هي القيمة الناتجة ؛ وإلا ، فإن السلوك غير محدد.

لذلك ، لا يتم تعريف السلوك إذا كان المعامل الأيمن أو الأيسر له قيمة سالبة. إذا كان المُعامل من نوع موقّع ، وقيمة غير سالبة ، ويلائم النوع الناتج ، فسيكون السلوك طبيعيًا.

جزء N2

V517استخدام نمط 'if (A) {...} آخر إذا تم الكشف عن (A) {...}'. هناك احتمال لوجود خطأ منطقي. تحقق من الخطوط: 156 ، 160. cameracapturelevelcontrol.cpp 156

void CameraCaptureLevelHistogram::mousePressEvent(QMouseEvent* event) {
  if (event->button() != Qt::LeftButton) return;
  if (m_currentItem == Histogram) {
    m_histogramCue = true;
    return;
  }
  if (m_currentItem == None) return;
  QPoint pos = event->pos();
  if (m_currentItem == BlackSlider)  // <=
    m_offset = pos.x() - SIDE_MARGIN - m_black;
  else if (m_currentItem == GammaSlider)
    m_offset = pos.x() - SIDE_MARGIN - gammaToHPos(m_gamma, m_black, m_white);
  else if (m_currentItem == BlackSlider)  // <=
    m_offset = pos.x() - SIDE_MARGIN - m_white;
  else if (m_currentItem == ThresholdSlider)
    m_offset = pos.x() - SIDE_MARGIN - m_threshold;
}

هنا يتم تعيين قيم مختلفة m_offset متغير اعتمادًا على قيمة m_currentItem . ومع ذلك ، فإن تكرار التحقق على BlackSlider لا طائل منه ، ومن نص الشرط يمكنك أن ترى أن المتغير m_white متضمن في الحساب . دعونا نلقي نظرة على القيم المحتملة لـ m_currentItem .

  LevelControlItem m_currentItem;

  enum LevelControlItem {
    None = 0,
    BlackSlider,
    WhiteSlider,
    GammaSlider,
    ThresholdSlider,
    Histogram,
    NumItems
  };

image3.png

اتضح أن قيمة WhiteSlider ممكنة أيضًا ، والتي لا يتم التحقق منها. وبالتالي ، من الممكن تمامًا أن تكون بعض سيناريوهات السلوك قد فقدت بسبب خطأ النسخ واللصق.

الجزء N3

V517 تم الكشف عن استخدام نمط 'if (A) {...} else if (A) {...}'. هناك احتمال لوجود خطأ منطقي. تحقق من الخطوط: 784 ، 867. tpalette.cpp 784

void TPalette::loadData(TIStream &is) {
  ....
  std::string tagName;
  while (is.openChild(tagName)) {
    if (tagName == "version") {
      ....
    } else if (tagName == "stylepages") { // <=
      while (!is.eos()) {
        if (....){
          ....
        }
        ....
        is.closeChild();
        }
    } else if (tagName == "refImgPath") {
      ....
    } else if (tagName == "animation") {
      ....
    } else if (tagName == "stylepages") { // <=
      int key = '0';
      while (!is.eos()) {
        int styleId = 0;
        ....
      }
    } 
      ....
  }
}

خطأ آخر مماثل. هنا ، نفس الظروف لها أجسام مختلفة ، ولكن لم يعد من الممكن الاستنتاج حول الخيارات الممكنة لقيمة tagName . على الأرجح ، تم فقد بعض الخيارات فقط ، وفي النهاية لدينا رمز لن يتم تنفيذه مطلقًا.

الجزء N4

V547 Expression 'chancount == 2' صحيح دائمًا. psd.cpp 720

void TPSDReader::readImageData(...., int chancount) {
  ....
  if (depth == 1 && chancount == 1) { // <= 1
    ....
  } else if (depth == 8 && chancount > 1) {
    ....
    for (....) {
      if (chancount >= 3) {
        ....
        if (chancount == 4)  
          ....
        else
          ....
      } else if (chancount <= 2)  // <= 2
      {
        ....
        if (chancount == 2) // <= 3
          ....
        else
          ....
      }
      ....
    }
    ....
  } else if (m_headerInfo.depth == 8 && chancount == 1) {
  ....
}

تسلل خطأ منطقي صغير في عمليات التحقق هذه. في الاختبار تحت رقم واحد ، تتم مقارنة حساب التغيير بالرقم 1 ، وفي الاختبار رقم 2 ، يتم التحقق من أن هذا المتغير أقل من أو يساوي 2. ونتيجة لذلك ، فإن قيمة التغيير المحتملة الوحيدة هي 2 للحالة تحت الرقم 3. قد لا يؤدي هذا الخطأ إلى تشغيل برنامج غير صحيح ، لكنه يعقد قراءة وفهم الشفرة. على سبيل المثال ، ليس من الواضح لماذا الفرع الآخر إذن ...

بشكل عام ، تأخذ الوظيفة التي تم النظر فيها في هذا الجزء أكثر من 300 سطر من التعليمات البرمجية وتتكون من أكوام من الشروط والحلقات.

image4.png

الجزء N5

V614 متغير غير مهيأ "precSegmentIndex" المستخدم. ضع في اعتبارك التحقق من الوسيطة الفعلية الخامسة لوظيفة 'insertBoxCorners'. 803

TStroke getIntersectedStroke(TStroke &stroke, TRectD bbox) {
  ....
  int precSegmentIndex, currentSegmentIndex, startSegmentIndex,
      precChunkIndex = -1;
  ....
  if (....) {
    insertBoxCorners(bbox, points, outPoints, currentSegmentIndex,
                     precSegmentIndex);
    ....
  }
}

void insertBoxCorners(...., int currentSegmentIndex, int precSegmentIndex) {
  ....
  bool sameIndex = (precSegmentIndex == currentSegmentIndex);
  ....
  int segmentIndex = precSegmentIndex;
  ....
}

ربما ، حدث خطأ هنا حتى أثناء تهيئة المتغيرات precSegmentIndex ، currentSegmentIndex ، startSegmentIndex ، precChunkIndex . يمكن للمطور أن يتوقع أن تهيئة العنصر الأخير -1 تتم تهيئته بنفس قيمة المتغيرات الأخرى المعلنة على نفس السطر.

الجزء N6

V590 فكّر في فحص تعبير 's! = "" && s == "color" ". التعبير زائد أو يحتوي على خطأ مطبعي. 416

void CleanupParameters::loadData(TIStream &is, bool globalParams) {
  ....
  std::string s = is.getTagAttribute("sharpness");
  ....
  if (....)
  {
    ....
  } else if (tagName == "lineProcessing")
    ....
    if (s != "" && isDouble(s)) 
      ....
    if (s != "" && isDouble(s))
      ....
    if (s != "" && s == "color") // <=
      ....
  } else if (tagName == "despeckling") {
    ....  
  }
  ....
}

هذا الخطأ ، حتى أنه عيب ، لا يؤدي إلا إلى مقارنة غير ضرورية. ومع ذلك ، إذا نظرنا إلى الشفرة ككل ، فسيكون من الواضح أن المقارنة الإضافية ظهرت نتيجة لصق النسخ من الشروط السابقة.

image5.png

قد تحتوي هذه المعكرونة ، التي تحتل عشرات الأسطر أو أكثر من التعليمات البرمجية ، على أي أخطاء منطقية أخرى ، ويمكن أن يتحول بحثها باستخدام هذا التنسيق إلى عذاب.

جزء N7

V772 استدعاء عامل "حذف" لمؤشر باطل سيؤدي إلى سلوك غير محدد. 1327

static void release_interface(void *interf) {
  if (interf) delete interf;
}

هنا رسالة محلل في حد ذاته هو بالفعل شاملة للغاية: استدعاء حذف المشغل على مؤشر إلى باطل نوع يؤدي إلى سلوك غير معرف. إذا كانت هناك حاجة إلى وظيفة عالمية لإزالة الواجهات ، فقد يكون من المفيد جعلها لوحة مرجعية.

template<class T>
static void release_interface(T *interf) {
  if (interf) delete interf;
}

جزء N8

V568 من الغريب أن العامل 'sizeof ()' يقيّم حجم المؤشر إلى فئة ، ولكن ليس حجم كائن فئة 'm_xshHandle'. 455

class DVAPI TStageObjectParams {
  ....
};

class RemovePegbarNodeUndo final : public TUndo {
  ....
  TXsheetHandle *m_xshHandle;

public:
  int getSize() const override {
    return sizeof *this + sizeof(TStageObjectParams) + sizeof(m_xshHandle);
  }
  ....
}

خطأ شائع بما فيه الكفاية يمكن أن يحدث من خلال عدم الانتباه والجهل. هنا ، على الأرجح ، كان الأمر غافلًا ، لأنه في الفصل الأول تم إلغاء هذا الأمر مع ذلك. إذا كنت بحاجة إلى حجم كائن ، يجب أن تتذكر دائمًا أنه يجب إلغاء الإشارة إلى مؤشر الكائن. خلاف ذلك ، نحصل فقط على حجم المؤشر نفسه.

return sizeof *this + sizeof(TStageObjectParams) + sizeof(*m_xshHandle);

جزء N9

V568 من الغريب أن العامل 'sizeof ()' يقيم حجم المؤشر إلى فئة ، ولكن ليس حجم كائن الفئة 'this'. shaderfx.cpp 107

struct RectF {
  GLfloat m_val[4];
  ....
  bool operator==(const RectF &rect) const {
    return (memcmp(m_val, rect.m_val, sizeof(this)) == 0);
  }
};

وهنا من الواضح أنهم نسوا الإشارة إلى هذا المؤشر . نتيجة لذلك ، لا نحصل على حجم الكائن ، ولكن حجم المؤشر. ونتيجة لذلك ، تتم مقارنة أول 4 أو 8 بايت فقط (اعتمادًا على عمق البت). النسخة الصحيحة من الكود:

return (memcmp(m_val, rect.m_val, sizeof(*this)) == 0);

الجزء N10

V554 الاستخدام غير الصحيح لـ unique_ptr. سيتم تنظيف الذاكرة المخصصة لـ "new []" باستخدام "delete". شاشة التوقف. cpp 29

void makeScreenSaver(TFilePath scrFn, TFilePath swfFn,
                     std::string screenSaverName) {
  struct _stat results;
....
  int swfSize = results.st_size;
  std::unique_ptr<char> swf(new char[swfSize]);
....
}

غالبًا ما يُنسى أن النوع الذي يتم من خلاله إنشاء unique_ptr يعتمد على ما إذا كان سيتم استخدام الحذف أو الحذف. ونتيجة لذلك ، إذا قمت بنسخ المؤشر كما هو الحال في الجزء قيد النظر ، أثناء تخصيص الذاكرة من خلال [] جديد ، فقد يحدث سلوك غير محدد ، لأن الإصدار سيحدث من خلال الحذف. لتجنب ذلك ، تحتاج إلى إضافة أقواس مربعة إلى نوع المؤشر: std :: unique_ptr <char []>.

الجزء N11

V521 مثل هذه التعبيرات التي تستخدم عامل التشغيل "،" خطيرة. تأكد من صحة التعبير 'm_to، m_from = it-> first.getNumber ()'. 509

class LoadImagesPopup final : public FileBrowserPopup {
  ....
  int m_from, m_to, ....;
  ....
}
void LoadImagesPopup::onFilePathClicked(....) {
  TLevel::Iterator it;
  ....
  it = level->begin();
  m_to, m_from = it->first.getNumber();  // <=
  for (; it != level->end(); ++it) m_to = it->first.getNumber();

  if (m_from == -2 && m_to == -2) m_from = m_to = 1;

  m_minFrame = m_from;
  m_maxFrame = m_to;
  ....
}

ربما توقع المبرمج أنه يمكنك تعيين قيمة واحدة للعديد من المتغيرات ببساطة عن طريق كتابتها بفاصلة. ومع ذلك ، يعمل عامل التشغيل "،" في C ++ بشكل مختلف. في ذلك ، يتم تنفيذ المعامل الأول ، وتكون النتيجة "إعادة تعيين" ، ثم يتم حساب المعامل الثاني. وعلى الرغم من أن المتغير m_to يتم تهيئته في دورة لاحقة ، إذا حدث خطأ ما أو إذا كان هناك شخص آخر يعمل بشكل غير دقيق ، فمن الممكن ألا يتلقى m_to قيمة. وبشكل عام ، على أي حال ، يبدو هذا الرمز غريبًا.

جزء N12

V532 ضع في اعتبارك فحص عبارة نمط * المؤشر +. يعني على الأرجح: "(* المؤشر) ++". تروب .140

template <class T, class Q>
void doGammaCorrect(TRasterPT<T> raster, double gamma) {
  Gamma_Lut<Q> lut(....);

  int j;
  for (j = 0; j < raster->getLy(); j++) {
    T *pix    = raster->pixels(j);
    T *endPix = pix + raster->getLx();
    while (pix < endPix) {
      pix->r = lut.m_table[pix->r];
      pix->b = lut.m_table[pix->b];
      pix->g = lut.m_table[pix->g];
      *pix++; // <=
    }
  }
}

عيب صغير ، يمكن أن يربك الشخص الذي يقرأ الرمز. تؤدي الزيادة ، كما هو مقصود ، إلى تحريك المؤشر. ومع ذلك ، بعد ذلك هناك عدم مرجعية لا معنى له. من الأفضل كتابة pix ++ فقط .

جزء N13

V773 تم إنهاء الوظيفة بدون تحرير مؤشر "autoCloseUndo". من الممكن حدوث تسرب للذاكرة. 575

void joinLineToLine(....) {
  ....
  UndoAutoclose *autoCloseUndo = 0;
  ....
  autoCloseUndo = new UndoAutoclose(....);
  ....
  if (pos < 0) return;
  ....
  TUndoManager::manager()->add(autoCloseUndo);
}

كان هناك أكثر من 20 من هذه التحذيرات ، وغالبًا ما يتم تحرير الذاكرة في مكان ما في نهاية الوظيفة ، ولكن يتم تخطي هذه الخطوة الضرورية للعودة المبكرة. لذا فهو هنا. في النهاية ، يتم تمرير المؤشر إلى TUndoManager :: manager () -> add () ، والذي يهتم بحذف المؤشر ، ولكن هناك عودة أعلاه نسيت أن تستدعي هذه الطريقة. لذلك من الجدير دائمًا تذكر مؤشراتك كلما خرجت من الوظيفة ، وليس فقط إدخال الحذف في مكان ما في نهاية الكتلة أو قبل العودة الأخيرة .

ومع ذلك ، إذا بدا هذا الخطأ واضحًا بالنسبة لإصدار مختصر من الشفرة ، فعند رمز حقيقي أكثر تعقيدًا ، قد يكون تعقب هذه المشكلة أمرًا صعبًا. هنا سوف يساعدك المحلل الثابت المتعب.

الجزء N14

V522 قد يتم إلغاء الإشارة إلى "منطقة" المؤشر الخالية. palettecmd.cpp 94

bool isStyleUsed(const TVectorImageP vi, int styleId) {
  ....
  int regionCount = vi->getRegionCount();
  for (i = 0; i < regionCount; i++) {
    TRegion *region = vi->getRegion(i);
    if (region || region->getStyle() != styleId) return true;
  }
  ....
}

هنا يمكننا أن نفترض أن المطور قام بخلط قواعد تقييم دائرة القصر ، واعتقد أنه إذا أعاد الفحص الأول للمؤشر خطأ ، فلن يحدث إلغاء الإشارة لهذا المؤشر الفارغ. ومع ذلك ، بالنسبة للعامل "||" بل على العكس تمامًا.

جزء N15

V561 ربما يكون من الأفضل تعيين قيمة لمتغير 'ca' بدلاً من إعلانها من جديد. إعلان السابق: xshcellmover.cpp، خط 319. xshcellmover.cpp 323

V561 انها على الارجح أفضل لقيمة تعيين إلى المتغير 'سي بي "من أن يعلن من جديد. الإعلان السابق: xshcellmover.cpp ، السطر 320. xshcellmover.cpp 324xshcellmover.cpp 323

void redo() const override {
  int ca       = m_cellsMover.getStartPos().x;
  int cb       = m_cellsMover.getPos().x;
  ....
  if (!m_cellsMover.getOrientation()->isVerticalTimeline()) {
    int ca = m_cellsMover.getStartPos().y;
    int cb = m_cellsMover.getPos().y;
  }
  ....
  if (ca != cb) {
    ....
  }
  ....
}

ربما نسخة أخرى للصق ، ولكن مع جوهر الخطأ ليس عاديًا جدًا. تم استبدال استدعاء x بـ y ، لكنهم نسوا إزالة نوع المتغير في بداية السطر ، والذي يحدث بسببه إعادة الإبلاغ المحلية. ونتيجة لذلك، بدلا من تغيير اتجاه موقف لالأولية كاليفورنيا و سي بي ، محلية جديدة كاليفورنيا و يتم إنشاؤها سي بي ، التي لم يحدث شيء من ذلك. لكن الخارجية كاليفورنيا و سي بي تستمر في الوجود مع قيم س .

الاستنتاج N1

في عملية كتابة المقال ، أصبح من المثير للاهتمام أن أقوم بمحاولة القيام بشيء ما في هذا البرنامج. ربما كنت محظوظا، ولكن سلوك غريب لم يمض وقت طويل في القادمة: معطلا، عرض بلدي التلاعب في قرص بعد تراجع وساحة الغريب عن طريق الضغط على مفتاح Ctrl + Z لل . لسوء الحظ ، لم أستطع تكرار هذا السلوك.

image6.png

ولكن في الواقع ، على الرغم من هذا السلوك وغرس عادة الضغط بانتظام على Ctrl + S ، فإن OpenToonz يثير الإعجاب بمقياسه ووظائفه. ومع ذلك ، ليس من دون جدوى أن الاستوديوهات الكبيرة تستخدمه أيضًا.

وفني كمكافأة:

image7.gif

الاستنتاج N2

في حالة OpenToonz ، من الواضح أن محاولة إصلاح جميع الأخطاء التي اكتشفها المحلل في وقت واحد ستكون مهمة كبيرة ستعطل عملية التطوير. بالنسبة لمثل هذه الحالات ، هناك نهج "قمع جماعي" ، عندما يتم إدخال الدين التقني في قاعدة المكثف للمحلل ويتم إجراء المزيد من العمل مع المحلل بناءً على استجابات جديدة. حسنًا ، إذا ظهر الوقت ، يمكنك فرز الديون الفنية.

ملاحظة: أذكركم بأن مطوري المشاريع مفتوحة المصدر يمكنهم استخدام خيار الترخيص المجاني PVS-Studio . إذا كنت ترغب في مشاركة هذه المقالة مع جمهور يتحدث الإنجليزية ، فيرجى استخدام رابط الترجمة: Victoria Khanieva. OpenToonz .




All Articles