ProHoster > Blog > Administrimi > Testimi i FreeRDP me ndihmën e analizatorit PVS-Studio

Testimi i FreeRDP me ndihmën e analizatorit PVS-Studio

Kontrollimi i FreeRDP me ndihmën e analizuesit PVS-Studio
FreeRDP – Ă«shtĂ« njĂ« implementim i hapur i Protokollit tĂ« Desktopit tĂ« LargĂ«t (RDP), njĂ« protokoll qĂ« mundĂ«son menaxhimin e largĂ«t tĂ« kompjuterĂ«ve, zhvilluar nga kompania Microsoft. Projekti mbĂ«shtet shumĂ« platforma, pĂ«rfshirĂ« Windows, Linux, macOS dhe madje edhe iOS me Android. Ky projekt Ă«shtĂ« zgjedhur si i pari nĂ« ciklin e artikujve qĂ« kushtohen kontrollit tĂ« klientĂ«ve RDP me ndihmĂ«n e analizatorit statik PVS-Studio.

Pak histori

Projekt FreeRDP u shfaq pasi Microsoft publikoi specifikimet e protokollit të saj pronësor RDP. Në atë kohë ekzistonte klienti rdesktop, implementimi i të cilit bazohej në rezultatet e inxhinierisë së kundërt.

GjatĂ« realizimit tĂ« protokollit, u bĂ« mĂ« e vĂ«shtirĂ« tĂ« shtohej funksionalitet i ri pĂ«r shkak tĂ« arkitekturĂ«s ekzistuese tĂ« projektit. Ndryshimet nĂ« tĂ« shkaktuan njĂ« konflikt midis zhvilluesve, çka çoi nĂ« krijimin e fork-ut rdesktop — FreeRDP. RrĂ«fimi mĂ« tej i produktit ishte i kufizuar nga licenca GPLv2, pĂ«r kĂ«tĂ« arsye u mor vendimi pĂ«r ri-licencimin nĂ«n Apache License v2. MegjithatĂ«, jo tĂ« gjithĂ« ishin dakord tĂ« ndĂ«rronin licencĂ«n e kodit tĂ« tyre, prandaj zhvilluesit vendosĂ«n ta ridizajnojnĂ« projektin, çka rezultoi nĂ« pamjen moderne tĂ« bazĂ«s sĂ« kodit.

Më shumë rreth historisë së projektit mund të lexoni në shënimin e blogut zyrtar: «Historia e projektit FreeRDP».

Si një mjet për identifikimin e gabimeve dhe potencialeve të cenueshmërisë në kod, është përdorur PVS-Studio. Ky është një analizues statik i kodit për gjuhët C, C++, C# dhe Java, i disponueshëm në platformat Windows, Linux dhe macOS.

Në këtë artikull paraqiten vetëm ato gabime që më dukeshin më interesante.

Memorie leaking

V773 Funksioni u mbyll pa liruar treguesin ‘cwd’. NjĂ« shpĂ«rthim memorie Ă«shtĂ« i mundshĂ«m. environment.c 84

DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
  char* cwd;
  ....
  cwd = getcwd(NULL, 0);
  ....
  if (lpBuffer == NULL)
  {
    free(cwd);
    return 0;
  }

  if ((length + 1) > nBufferLength)
  {
    free(cwd);
    return (DWORD) (length + 1);
  }

  memcpy(lpBuffer, cwd, length + 1);
  return length;
  ....
}

Kyç këtu është marrë nga sistemi winpr, që zbaton një paketë WINAPI për sistemet jo-Windows, domethënë, është një ekvivalent i lehtë i Wine. Këtu mund të vërehet një rrjedhje: memoria e alokuar nga funksioni getcwd, çlirohet vetëm kur trajtohen raste speciale. Për të eliminuar gabimin, duhet të shtoni thirrjen free pas memcpy.

Shkëputje jashtë kufijve të array

V557 ËshtĂ« e mundur njĂ« rrjedhje nga array. Vlera e indekse ‘event->EventHandlerCount’ mund tĂ« arrijĂ« 32. PubSub.c 117

#define MAX_EVENT_HANDLERS  32

struct _wEventType
{
  ....
  int EventHandlerCount;
  pEventHandler EventHandlers[MAX_EVENT_HANDLERS];
};

int PubSub_Subscribe(wPubSub* pubSub, const char* EventName,
      pEventHandler EventHandler)
{
  ....
  if (event->EventHandlerCount <= MAX_EVENT_HANDLERS)
  {
    event->EventHandlers[event->EventHandlerCount] = EventHandler;
    event->EventHandlerCount++;
  }
  ....
}

Në këtë shembull, një element i ri shtohet në listë, edhe nëse numri i elementeve ka arritur maksimumin. Mjafton të zëvendësoni operatorin <= në <, për të mos kaluar kufijtë e array.

Një gabim tjetër i këtij tipi u gjet:

  • V557 ËshtĂ« e mundur njĂ« rrjedhje nga array. Vlera e indekse ‘iBitmapFormat’ mund tĂ« arrijĂ« 8. orders.c 2623

Gabime shkrimi

Fragmenti 1

V547 Shprehja ‘!pipe->In’ Ă«shtĂ« gjithmonĂ« false. MessagePipe.c 63

wMessagePipe* MessagePipe_New()
{
  ....
  pipe->In = MessageQueue_New(NULL);
  if (!pipe->In)
    goto error_in;

  pipe->Out = MessageQueue_New(NULL);
  if (!pipe->In) // <=
    goto error_out;
  ....
}

Këtu shohim një gabim të zakonshëm: në kushtin e dytë kontrollohet e njëjta variabël si në të parin. Probabiliteti është që gabimi ndodhi si rezultat i kopjimit të dështëpër të kodit.

Fragmenti 2

V760 U gjetën dy blloqe identike teksti. Blloku i dytë fillon nga linja 771. tsg.c 770

typedef struct _TSG_PACKET_VERSIONCAPS
{
  ....
  UINT16 majorVersion;
  UINT16 minorVersion;
  ....
} TSG_PACKET_VERSIONCAPS, *PTSG_PACKET_VERSIONCAPS;

static BOOL TsProxyCreateTunnelReadResponse(....)
{
  ....
  PTSG_PACKET_VERSIONCAPS versionCaps = NULL;
  ....
  /* MajorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  /* MinorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  ....
}

Një tjetër gabim: komentari i kodit sugjeron se nga rrjedha duhet të vijë minorVersion, megjithatë leximi ndodh në një variabël me emrin majorVersion. Megjithatë, nuk jam i njohur me protokollin, kështu që kjo është thjesht një supozim.

Fragmenti 3

V524 ËshtĂ« çuditshĂ«m qĂ« trupi i funksionit ‘trio_index_last’ Ă«shtĂ« plotĂ«sisht ekuivalent me trupin e funksionit ‘trio_index’. triostr.c 933

/**
   Find first occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

/**
   Find last occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

Sipas komentit, funksioni trio_index gjen shkallĂ«n e parĂ« tĂ« pĂ«rputhjes sĂ« karakterit nĂ« varg, ndĂ«rsa trio_index_last — e fundit. Por trupat e kĂ«tyre funksioneve janĂ« identike! Probabiliteti Ă«shtĂ« se Ă«shtĂ« njĂ« gabim, dhe nĂ« funksionin trio_index_last duhet tĂ« pĂ«rdoret strrchr nĂ« vend tĂ« strchr. AtĂ«herĂ« sjellja do tĂ« jetĂ« siç pritet.

Fragmenti 4

V769 Pika ‘data’ nĂ« kĂ«tĂ« shprehje Ă«shtĂ« baraz me nullptr. Vlera e rezultateve nga operacionet aritmetike mbi kĂ«tĂ« tregues nuk ka kuptim dhe nuk duhet pĂ«rdorur. nsc_encode.c 124

static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
                                      const BYTE* data,
                                      UINT32 scanline)
{
  ....
  if (!context || data || (scanline == 0))
    return FALSE;
  ....
  src = data + (context->height - 1 - y) * scanline;
  ....
}

Duket se Ă«shtĂ« harruar aksioni i mohimit kĂ«tu rastĂ«sisht ! pranĂ« data. ÇuditĂ«risht, qĂ« kjo ka mbetur e papĂ«rmendur.

Fragmenti 5

V517 ËshtĂ« zbuluar modeli ‘if (A) {
} else if (A) {
}’. Ka njĂ« probabilitet pĂ«r praninĂ« e gabimit logjik. Kontrolloni linjat: 213, 222. rdpei_common.c 213

BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
  BYTE byte;

  if (value <= 0x3F)
  {
    ....
  }
  else if (value <= 0x3FFF)
  {
    ....
  }
  else if (value > 16) & 0x3F;
    Stream_Write_UINT8(s, byte | 0x80);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  else if (value > 24) & 0x3F;
    Stream_Write_UINT8(s, byte | 0xC0);
    byte = (value >> 16) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  ....
}

Këto kushte të fundit janë të njëjta: duket se dikush ka harruar t'i kontrollojë ato pas kopjimit. Nga kodi, është e dukshme se pjesa e fundit punon me vlera katërbajtëshe, prandaj mund të supozojmë se kushti i fundit duhet të jetë value <= 0x3FFFFFFF.

Një gabim tjetër i këtij tipi u gjet:

  • V517 U zbulua modeli 'if (A) {
} else if (A) {
}'. Ka njĂ« probabilitet tĂ« pranishĂ«m tĂ« gabimeve logjike. Kontrolloni linjat: 169, 173. file.c 169

Kontrollimi i të dhënave hyrëse

Fragmenti 1

V547 Shprehja ‘strcat(target, source) != NULL’ Ă«shtĂ« gjithmonĂ« e vĂ«rtetĂ«. triostr.c 425

TRIO_PUBLIC_STRING int
trio_append
TRIO_ARGS2((target, source),
     char *target,
     TRIO_CONST char *source)
{
  assert(target);
  assert(source);
  
  return (strcat(target, source) != NULL);
}

Kontrollimi i rezultatit të ekzekutimit të funksionit në këtë shembull është i pasaktë. Funksioni strcat kthen një tregues në variantin përfundimtar të vargjes, domethënë parametrin e parë të kaluar. Në këtë rast, ky është target. Por nëse ai është i barabartë me NULL, atëherë kontrollimi i tij është tepër vonë, pasi në funksionin strcat do të ndodhë dereferencimi.

Fragmenti 2

V547 Shprehja ‘cache’ Ă«shtĂ« gjithmonĂ« e vĂ«rtetĂ«. glyph.c 730

typedef struct rdp_glyph_cache rdpGlyphCache;

struct rdp_glyph_cache
{
  ....
  GLYPH_CACHE glyphCache[10];
  ....
};

void glyph_cache_free(rdpGlyphCache* glyphCache)
{
  ....
  GLYPH_CACHE* cache = glyphCache->glyphCache;

  if (cache)
  {
    ....
  }
  ....
}

Në këtë rast, variabli cache i jepet adresa e një array statik glyphCache->glyphCache. Në këtë mënyrë, kontrolli if (cache) mund të lihet

Gabim në menaxhimin e burimeve

V1005 Burimi u marrĂ« duke pĂ«rdorur funksionin ‘CreateFileA’ por u lirua me funksionin e papĂ«rputhshĂ«m ‘fclose’. certificate.c 447

BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
                              rdpCertificateData* certificate_data)
{
  HANDLE fp;
  ....
  fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
                   NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  ....
  if (size < 1)
  {
    CloseHandle(fp);
    return FALSE;
  }
  ....
  if (!data)
  {
    fclose(fp);
    return FALSE;
  }
  ....
}

PĂ«rshkrimi i skedarit fp, i krijuar nga thirrja e funksionit CreateFile, ka mbyllur gabimisht me funksionin fclose nga biblioteka standarde, dhe jo CloseHandle.

Kushtet e njëjta

V581 Shprehjet kushtore tĂ« deklaratave ‘if’ tĂ« vendosura pĂ«rkrah njĂ«ra-tjetrĂ«s janĂ« identike. Kontrolloni linjat: 269, 283. ndr_structure.c 283

void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
      unsigned char* pMemory, PFORMAT_STRING pFormat)
{
  ....
  if (conformant_array_description)
  {
    ULONG size;
    unsigned char array_type;
    array_type = conformant_array_description[0];
    size = NdrComplexStructMemberSize(pStubMsg, pFormat);
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
    NdrpComputeConformance(pStubMsg, pMemory + size,
      conformant_array_description);
    NdrpComputeVariance(pStubMsg, pMemory + size,
      conformant_array_description);
    MaxCount = pStubMsg->MaxCount;
    ActualCount = pStubMsg->ActualCount;
    Offset = pStubMsg->Offset;
  }

  if (conformant_array_description)
  {
    unsigned char array_type;
    array_type = conformant_array_description[0];
    pStubMsg->MaxCount = MaxCount;
    pStubMsg->ActualCount = ActualCount;
    pStubMsg->Offset = Offset;
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
  }
  ....
}

Ndoshta ky shembull nuk është një gabim. Sidoqoftë, të dy kushtet përmbajnë të njëjtat mesazhe, njëra prej të cilave ndoshta mund të hiqet.

Pastrimi i treguesve nul

V575 Treguesi nul kalon nĂ« funksionin ‘free’. Kontrolloni argumentin e parĂ«. smartcard_pcsc.c 875

WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
  SCARDCONTEXT hContext,
  LPCWSTR mszGroups,
  LPWSTR mszReaders,
  LPDWORD pcchReaders)
{
  LPSTR mszGroupsA = NULL;
  ....
  mszGroups = NULL; /* mszGroups is not supported by pcsc-lite */

  if (mszGroups)
    ConvertFromUnicode(CP_UTF8,0, mszGroups, -1, 
                       (char**) &mszGroupsA, 0,
                       NULL, NULL);

  status = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
                                          (LPSTR) &mszReadersA,
                                          pcchReaders);

  if (status == SCARD_S_SUCCESS)
  {
    ....
  }

  free(mszGroupsA);
  ....
}

Në funksionin free mund të kalojë një tregues të null dhe analizatori e di këtë. Por nëse ndodh një situatë ku treguesi gjithmonë kalon si null, siç është në këtë fragment, do të lëshohet një paralajmërim.

Tregues mszGroupsA fillimisht është e barabartë NULL dhe nuk inicializohet askund tjetër. Dega e vetme e kodit ku treguesi mund të inicializohet është e paarritshme.

Ishin edhe mesazhe të tjera të këtij lloji:

  • V575 Treguesi null po kalon nĂ« funksionin ‘free’. Kontrolloni argumentin e parĂ«. license.c 790
  • V575 Treguesi null po kalon nĂ« funksionin ‘free’. Kontrolloni argumentin e parĂ«. rdpsnd_alsa.c 575

Më së shpeshti, këto variabla të harruara ndodhin gjatë procesit të ristrukturimit dhe mund të fshihen lehtësisht.

Përgjithësia e mundshme

V1028 Përgjithësi e mundshme. Merrni parasysh hedhjen e operandëve, jo rezultatit. makecert.c 1087

// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);

struct _MAKECERT_CONTEXT
{
  ....
  int duration_years;
  int duration_months;
};

typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;

int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
  ....
  if (context->duration_months)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
      context->duration_months));
  else if (context->duration_years)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
      context->duration_years));
  ....
}

Hedhja e rezultatit në long nuk është mbrojtje nga mbipërçimi, sepse vetë llogaritja ndodh duke përdorur tipin int.

Zhbllokimi i treguesit në inicializim

V595 Treguesi ‘context’ u shfrytĂ«zua para se tĂ« verifikohej nĂ« lidhje me nullptr. Kontrolloni linjat: 746, 748. gfx.c 746

static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
                               const RDPGFX_SURFACE_COMMAND* cmd)
{
  ....
  rdpGdi* gdi = (rdpGdi*) context->custom;

  if (!context || !cmd)
    return ERROR_INVALID_PARAMETER;
  ....
}

KĂ«tu treguesi context shkĂ«putet nĂ« inicializim — pĂ«rpara se tĂ« kontrollohet.

U gjetën edhe gabime të tjera të këtij lloji:

  • V595 Treguesi ‘ntlm’ u shfrytĂ«zua para se tĂ« verifikohej nĂ« lidhje me nullptr. Kontrolloni linjat: 236, 255. ntlm.c 236
  • V595 Treguesi ‘context’ u shfrytĂ«zua para se tĂ« verifikohej nĂ« lidhje me nullptr. Kontrolloni linjat: 1003, 1007. rfx.c 1003
  • V595 Treguesi ‘rdpei’ u shfrytĂ«zua para se tĂ« verifikohej nĂ« lidhje me nullptr. Kontrolloni linjat: 176, 180. rdpei_main.c 176
  • V595 Treguesi ‘gdi’ u shfrytĂ«zua para se tĂ« verifikohej nĂ« lidhje me nullptr. Kontrolloni linjat: 121, 123. xf_gfx.c 121

Kusht i paarsyeshëm

V547 Shprehja ‘rdp->state >= CONNECTION_STATE_ACTIVE’ Ă«shtĂ« gjithmonĂ« e vĂ«rtetĂ«. connection.c 1489

int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
  ....
  switch (state)
  {
    ....
    case CONNECTION_STATE_ACTIVE:
      rdp->state = CONNECTION_STATE_ACTIVE;          // state >= CONNECTION_STATE_ACTIVE)     // Activate, client->activated, client);

        if (!client->activated)
          return -1;
      }
    ....
  }
  ....
}

ËshtĂ« e lehtĂ« tĂ« vĂ«resh se kushti i parĂ« nuk ka kuptim pĂ«r shkak tĂ« caktimit tĂ« vlerĂ«s pĂ«rkatĂ«se mĂ« parĂ«.

Analizë e gabuar e vargut

V576 Format i gabuar. Merrni parasysh kontrollimin e argumentit tĂ« tretĂ« aktual tĂ« funksionit ‘sscanf’. Pritet njĂ« tregues pĂ«r tipin unsigned int. proxy.c 220

V560 Një pjesë e shprehjes kushtore është gjithmonë e vërtetë: (rc >= 0). proxy.c 222

static BOOL check_no_proxy(....)
{
  ....
  int sub;
  int rc = sscanf(range, "%u", &sub);

  if ((rc == 1) && (rc >= 0))
  {
    ....
  }
  ....
}

Analizatori për këtë fragment jep menjëherë 2 paralajmërime. Specifikatori %u pret një variabël të tipit unsigned int, por variabli sub ka tipin int. Më pas shohim një kontroll të dyshimtë: kushti përtej nuk ka kuptim, pasi fillimisht bëhet krahasimi me një.

Kontrolle të paorganizuar

V547 Shprehja ‘status == 0x00090314’ Ă«shtĂ« gjithmonĂ« e gabuar. ntlm.c 299

BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
  ....
  if (status != SEC_E_OK)
  {
    ....
    return FALSE;
  }

  if (status == SEC_I_COMPLETE_NEEDED)            // <=
    status = SEC_E_OK;
  else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
    status = SEC_I_CONTINUE_NEEDED;
  ....
}

Kushtet e shënuara do të jenë gjithmonë të falsifikuara, pasi ekzekutimi do të arrijë në kushtin e dytë vetëm nëse status == SEC_E_OK. Kodi i duhur mund të duket kështu:

nëse (statusi == SEC_I_COMPLETE_NEEDED)
  statusi = SEC_E_OK;
ndryshe nëse (statusi == SEC_I_COMPLETE_AND_CONTINUE)
  statusi = SEC_I_CONTINUE_NEEDED;
ndryshe nëse (statusi != SEC_E_OK)
{
  ....
  kthe FALSE;
}

PĂ«rfundimi

Kështu, kontrolli i projektit zbuloi shumë probleme, por vetëm pjesa më interesante e tyre u përshkrua në artikull. Zhvilluesit e projektit mund ta kontrollojnë vetë projektin, duke kërkuar një çelës përkohës në faqen e internetit. PVS-Studio. Kishte edhe njoftime të rrema, puna mbi të cilat do të ndihmojë për të përmirësuar analizuesin. Megjithatë, analiza statike është e rëndësishme nëse dëshironi jo vetëm të përmirësoni cilësinë e kodit, por edhe të shkurtoni kohën për të gjetur gabime, dhe PVS-Studio mund të ndihmojë për këtë.

Kontrollimi i FreeRDP me ndihmën e analizuesit PVS-Studio

Nëse dëshironi ta ndani këtë artikull me audiencën anglishtfolëse, ju lutem përdorni lidhjen për përkthimin: Sergey Larin. Kontrollimi i FreeRDP me PVS-Studio

Burimi: habr.com

Bleni hostim tĂ« besueshĂ«m pĂ«r faqe me mbrojtje nga DDoS, serverĂ« VPS VDS đŸ”„ Bleni hostim tĂ« besueshĂ«m pĂ«r faqe me mbrojtje nga DDoS, serverĂ« VPS VDS | ProHoster