التحقق من rdesktop و xrdp باستخدام محلل PVS-Studio

هذه هي المراجعة الثانية في سلسلة من المقالات حول اختبار البرامج مفتوحة المصدر للعمل مع بروتوكول RDP. سننظر فيه إلى عميل rdesktop وخادم xrdp.

تستخدم كأداة لتحديد الأخطاء استوديو PVS. وهو عبارة عن محلل أكواد ثابت للغات C وC++ وC# وJava، وهو متوفر على منصات Windows وLinux وmacOS.

يعرض المقال فقط تلك الأخطاء التي بدت مثيرة للاهتمام بالنسبة لي. لكن المشاريع صغيرة لذا كانت الأخطاء قليلة :).

لاحظ. يمكن العثور على مقالة سابقة حول التحقق من مشروع FreeRDP هنا.

rdesktop

rdesktop - تطبيق مجاني لعميل RDP للأنظمة المستندة إلى UNIX. يمكن استخدامه أيضًا ضمن Windows إذا قمت بإنشاء المشروع ضمن Cygwin. مرخص تحت GPLv3.

يحظى هذا العميل بشعبية كبيرة - فهو يُستخدم افتراضيًا في ReactOS، ويمكنك أيضًا العثور على واجهات أمامية رسومية تابعة لجهات خارجية له. ومع ذلك، فهو كبير في السن: فقد تم إطلاق سراحه لأول مرة في 4 أبريل/نيسان 2001 - وكان عمره وقت كتابة هذا التقرير 17 عامًا.

كما أشرت سابقًا، المشروع صغير جدًا. يحتوي على ما يقرب من 30 ألف سطر من التعليمات البرمجية، وهو أمر غريب بعض الشيء بالنظر إلى عمره. وللمقارنة فإن FreeRDP يحتوي على 320 ألف سطر. هنا هو إخراج برنامج Cloc:

رمز لا يمكن الوصول إليه

V779 تم اكتشاف رمز غير متاح. من الممكن أن يكون هناك خطأ. rdesktop.c 1502

int
main(int argc, char *argv[])
{
  ....
  return handle_disconnect_reason(deactivated, ext_disc_reason);

  if (g_redirect_username)
    xfree(g_redirect_username);

  xfree(g_username);
}

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

لا يوجد معالجة للأخطاء

V557 صفيف التجاوز ممكن. يمكن أن تصل قيمة المؤشر 'n' إلى -1. rdesktop.c 1872

RD_BOOL
subprocess(char *const argv[], str_handle_lines_t linehandler, void *data)
{
  int n = 1;
  char output[256];
  ....
  while (n > 0)
  {
    n = read(fd[0], output, 255);
    output[n] = ' '; // <=
    str_handle_lines(output, &rest, linehandler, data);
  }
  ....
}

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

استخدام EOF في نوع char

V739 لا ينبغي مقارنة EOF بقيمة من النوع "char". يجب أن يكون '(c = fgetc(fp))' من النوع 'int'. السيطرة.ج 500

int
ctrl_send_command(const char *cmd, const char *arg)
{
  char result[CTRL_RESULT_SIZE], c, *escaped;
  ....
  while ((c = fgetc(fp)) != EOF && index < CTRL_RESULT_SIZE && c != 'n')
  {
    result[index] = c;
    index++;
  }
  ....
}

هنا نرى معالجة غير صحيحة للوصول إلى نهاية الملف: if fgetc بإرجاع حرف رمزه هو 0xFF، وسيتم تفسيره على أنه نهاية الملف (EOF).

EOF وهو ثابت، يتم تعريفه عادةً على أنه -1. على سبيل المثال، في ترميز CP1251، يحتوي الحرف الأخير من الأبجدية الروسية على الرمز 0xFF، والذي يتوافق مع الرقم -1 إذا كنا نتحدث عن متغير مثل دبابة. وتبين أن الرمز 0xFF، مثل EOF (-1) يتم تفسيره على أنه نهاية الملف. لتجنب مثل هذه الأخطاء، تكون نتيجة الدالة fgetc يجب أن يتم تخزينها في متغير مثل مادبا.

الأخطاء المطبعية

جزء 1

V547 التعبير 'write_time' خطأ دائمًا. القرص ج 805

RD_NTSTATUS
disk_set_information(....)
{
  time_t write_time, change_time, access_time, mod_time;
  ....
  if (write_time || change_time)
    mod_time = MIN(write_time, change_time);
  else
    mod_time = write_time ? write_time : change_time; // <=
  ....
}

ربما أخطأ مؤلف هذا الكود || и && في حالة. دعونا نفكر في الخيارات الممكنة للقيم وقت الكتابة и غير الوقت:

• كلا المتغيرين يساوي 0: في هذه الحالة سننتهي في الفرع آخر: عامل mod_time سيكون دائمًا 0 بغض النظر عن الشرط اللاحق.
• أحد المتغيرات هو 0: mod_time ستكون مساوية للصفر (بشرط أن يكون للمتغير الآخر قيمة غير سالبة)، لأن دقيقة سيختار الأصغر من الخيارين.
• كلا المتغيرين لا يساويان 0: اختر القيمة الدنيا.

عند استبدال الشرط ب وقت الكتابة && وقت التغيير سيبدو السلوك صحيحًا:

• أحد المتغيرين أو كليهما لا يساوي 0: اختر قيمة غير الصفر.
• كلا المتغيرين لا يساويان 0: اختر القيمة الدنيا.

جزء 2

V547 التعبير صحيح دائما. ربما يجب استخدام عامل التشغيل "&&" هنا. القرص ج 1419

static RD_NTSTATUS
disk_device_control(RD_NTHANDLE handle, uint32 request, STREAM in,
      STREAM out)
{
  ....
  if (((request >> 16) != 20) || ((request >> 16) != 9))
    return RD_STATUS_INVALID_PARAMETER;
  ....
}

يبدو أن المشغلين مختلطون هنا أيضًا || и &&أو == и !=: لا يمكن أن يكون للمتغير القيمة 20 و9 في نفس الوقت.

نسخ سطر غير محدود

V512 سيؤدي استدعاء الدالة "sprintf" إلى تجاوز سعة المخزن المؤقت "fullpath". القرص ج 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Get information for directory entry */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

عندما تنظر إلى الوظيفة بالكامل، سيتبين لك أن هذا الكود لا يسبب مشاكل. ومع ذلك، قد تنشأ في المستقبل: تغيير واحد مهمل وسنحصل على تجاوز سعة المخزن المؤقت - sprintf لا يقتصر على أي شيء، لذلك عند تسلسل المسارات يمكننا تجاوز حدود المصفوفة. يوصى بملاحظة هذه المكالمة snprintf (المسار الكامل، PATH_MAX، ….).

حالة زائدة عن الحاجة

V560 جزء من التعبير الشرطي يكون صحيحًا دائمًا: أضف > 0. Scard.c 507

static void
inRepos(STREAM in, unsigned int read)
{
  SERVER_DWORD add = 4 - read % 4;
  if (add < 4 && add > 0)
  {
    ....
  }
}

Проверка إضافة > 0 ليست هناك حاجة هنا: المتغير سيكون دائما أكبر من الصفر، لأن قراءة %4 سيعيد باقي القسمة، لكنه لن يساوي 4 أبدًا.

xrdp

xrdp — تنفيذ خادم RDP بكود مفتوح المصدر. ينقسم المشروع إلى قسمين:

• xrdp - تنفيذ البروتوكول. وزعت بموجب ترخيص أباتشي 2.0.
• xorgxrdp - مجموعة من برامج تشغيل Xorg للاستخدام مع xrdp. الترخيص - X11 (مثل معهد ماساتشوستس للتكنولوجيا، ولكنه يحظر استخدامه في الإعلانات)

يعتمد تطوير المشروع على نتائج rdesktop وFreeRDP. في البداية، للعمل مع الرسومات، كان عليك استخدام خادم VNC منفصل، أو خادم X11 خاص مع دعم RDP - X11rdp، ولكن مع ظهور xorgxrdp، اختفت الحاجة إليهم.

في هذه المقالة لن نغطي xorgxrdp.

مشروع xrdp مثل المشروع السابق صغير جدًا ويحتوي على ما يقرب من 80 ألف سطر.

المزيد من الأخطاء المطبعية

V525 يحتوي الكود على مجموعة من الكتل المماثلة. تحقق من العناصر "r" و"g" و"r" في السطور 87 و88 و89.rfxencode_rgb_to_yuv.c 87

static int
rfx_encode_format_rgb(const char *rgb_data, int width, int height,
                      int stride_bytes, int pixel_format,
                      uint8 *r_buf, uint8 *g_buf, uint8 *b_buf)
{
  ....
  switch (pixel_format)
  {
    case RFX_FORMAT_BGRA:
      ....
      while (x < 64)
      {
          *lr_buf++ = r;
          *lg_buf++ = g;
          *lb_buf++ = r; // <=
          x++;
      }
      ....
  }
  ....
}

تم أخذ هذا الرمز من مكتبة librfxcodec، التي تطبق برنامج الترميز jpeg2000 لـ RemoteFX. هنا، على ما يبدو، يتم خلط قنوات البيانات الرسومية - بدلا من اللون "الأزرق" يتم تسجيل "الأحمر". على الأرجح ظهر هذا الخطأ نتيجة للنسخ واللصق.

حدثت نفس المشكلة في وظيفة مشابهة rfx_encode_format_argbوالذي أخبرنا به المحلل أيضًا:

V525 يحتوي الكود على مجموعة من الكتل المماثلة. تحقق من العناصر "a" و"r" و"g" و"r" في السطور 260 و261 و262 و263. rfxencode_rgb_to_yuv.c 260

while (x < 64)
{
    *la_buf++ = a;
    *lr_buf++ = r;
    *lg_buf++ = g;
    *lb_buf++ = r;
    x++;
}

إعلان المصفوفة

V557 تجاوز المصفوفة أمر ممكن. يمكن أن تصل قيمة الفهرس "i — 8" إلى 129. genkeymap.c 142

// evdev-map.c
int xfree86_to_evdev[137-8+1] = {
  ....
};

// genkeymap.c
extern int xfree86_to_evdev[137-8];

int main(int argc, char **argv)
{
  ....
  for (i = 8; i <= 137; i++) /* Keycodes */
  {
    if (is_evdev)
        e.keycode = xfree86_to_evdev[i-8];
    ....
  }
  ....
}

تعريف وتعريف المصفوفة في هذين الملفين غير متوافقين - يختلف الحجم بمقدار 1. ومع ذلك، لا تحدث أخطاء - تم تحديد الحجم الصحيح في ملف evdev-map.c، لذلك لا يوجد خارج الحدود. إذن هذا مجرد خطأ يمكن إصلاحه بسهولة.

مقارنة غير صحيحة

V560 جزء من التعبير الشرطي يكون دائمًا خطأ: (cap_len <0). xrdp_caps.c 616

// common/parse.h
#if defined(B_ENDIAN) || defined(NEED_ALIGN)
#define in_uint16_le(s, v) do 
....
#else
#define in_uint16_le(s, v) do 
{ 
    (v) = *((unsigned short*)((s)->p)); 
    (s)->p += 2; 
} while (0)
#endif

int
xrdp_caps_process_confirm_active(struct xrdp_rdp *self, struct stream *s)
{
  int cap_len;
  ....
  in_uint16_le(s, cap_len);
  ....
  if ((cap_len < 0) || (cap_len > 1024 * 1024))
  {
    ....
  }
  ....
}

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

الشيكات غير الضرورية

V560 جزء من التعبير الشرطي يكون صحيحًا دائمًا: (bpp != 16). libxrdp.c 704

int EXPORT_CC
libxrdp_send_pointer(struct xrdp_session *session, int cache_idx,
                     char *data, char *mask, int x, int y, int bpp)
{
  ....
  if ((bpp == 15) && (bpp != 16) && (bpp != 24) && (bpp != 32))
  {
      g_writeln("libxrdp_send_pointer: error");
      return 1;
  }
  ....
}

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

اختتام

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

يمكنك تنزيل نسخة تجريبية من PVS-Studio منا على الانترنت.

إذا كنت ترغب في مشاركة هذه المقالة مع جمهور ناطق باللغة الإنجليزية، يرجى استخدام رابط الترجمة: سيرجي لارين. التحقق من rdesktop وxrdp باستخدام PVS-Studio

المصدر: www.habr.com