בדיקת rdesktop ו-xrdp באמצעות מנתח PVS-Studio

זוהי הסקירה השנייה בסדרת מאמרים על בדיקת תוכנות קוד פתוח לעבודה עם פרוטוקול RDP. בו נסתכל על לקוח rdesktop ושרת xrdp.

משמש ככלי לזיהוי שגיאות סטודיו PVS. זהו מנתח קוד סטטי לשפות C, C++, C# ו-Java, זמין בפלטפורמות Windows, Linux ו-macOS.

המאמר מציג רק את השגיאות שנראו לי מעניינות. עם זאת, הפרויקטים קטנים, אז היו מעט טעויות :).

שים לב. ניתן למצוא מאמר קודם על אימות פרויקט FreeRDP כאן.

שולחן עבודה r

שולחן עבודה r - הטמעה חופשית של לקוח 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'. ctrl.c 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 אם אנחנו מדברים על משתנה כמו char. מסתבר שהסמל 0xFF, כמו EOF (-1) מתפרש כסוף הקובץ. כדי למנוע שגיאות כאלה, התוצאה של הפונקציה היא fgetc צריך להיות מאוחסן במשתנה כמו int.

שגיאות כתיב

קטע 1

V547 הביטוי 'זמן_כתיבה' הוא תמיד שקר. disk.c 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 (בתנאי שלמשתנה השני יש ערך לא שלילי), כי MIN יבחר בקטנה מבין שתי האפשרויות.
• שני המשתנים אינם שווים ל-0: בחר את הערך המינימלי.

כאשר מחליפים את המצב ב כתב_זמן && שינוי_זמן ההתנהגות תיראה נכונה:

• אחד המשתנים או שניהם אינם שווים ל-0: בחר ערך שאינו אפס.
• שני המשתנים אינם שווים ל-0: בחר את הערך המינימלי.

קטע 2

V547 ביטוי הוא תמיד נכון. כנראה יש להשתמש באופרטור '&&' כאן. disk.c 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'. disk.c 1257

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

כשתסתכלו על הפונקציה במלואה, יתברר שקוד זה אינו גורם לבעיות. עם זאת, הם עשויים להתעורר בעתיד: שינוי אחד רשלני ונקבל הצפת חיץ - ספרינטף אינו מוגבל בשום דבר, כך שכאשר משרשרים נתיבים אנו יכולים לחרוג מגבולות המערך. מומלץ לשים לב לקריאה זו snprintf(fullpath, 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 בקוד פתוח. הפרויקט מחולק ל-2 חלקים:

• xrdp - יישום פרוטוקול. מופץ תחת רישיון Apache 2.0.
• xorgxrdp - קבוצה של מנהלי התקנים של Xorg לשימוש עם xrdp. רישיון - X11 (כמו MIT, אך אוסר על שימוש בפרסום)

פיתוח הפרויקט מבוסס על תוצאות 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, המיישמת את ה-codec 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))
  {
    ....
  }
  ....
}

הפונקציה קוראת משתנה סוג לא חתום קצר לתוך משתנה כמו int. אין צורך בבדיקה כאן מכיוון שאנו קוראים משתנה ללא סימן ומקצים את התוצאה למשתנה גדול יותר, כך שהמשתנה לא יכול לקבל ערך שלילי.

בדיקות מיותרות

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