ProHoster > Блог > Administrado > Kontrolante FreeRDP per la analizilo PVS-Studio

Kontrolante FreeRDP per la analizilo PVS-Studio

Kontrolante FreeRDP per la analizilo PVS-Studio
FreeRDP estas malfermfonteca efektivigo de la Remote Desktop Protocol (RDP), protokolo evoluigita de Microsoft por fora komputila kontrolo. La projekto subtenas plurajn platformojn, inkluzive de Windows, Linux, macOS kaj eĉ iOS kun AndroidĈi tiu projekto estis elektita kiel la unua en serio de artikoloj dediĉitaj al testado de RDP-klientoj uzante la statikan analizilon PVS-Studio.

Iom da historio

La projekto FreeRDP okazis post kiam Mikrosofto malfermis la specifojn por sia proprieta RDP-protokolo. En tiu tempo, ekzistis rdesktop-kliento, kies efektivigo baziĝis sur la rezultoj de Reverse Engineering.

Ĉar la protokolo estis efektivigita, iĝis pli malfacile aldoni novan funkciecon pro la tiama ekzistanta projektarkitekturo. Ŝanĝoj en ĝi kaŭzis konflikton inter programistoj, kiu kaŭzis la kreadon de forko de rdesktop - FreeRDP. Plia distribuado de la produkto estis limigita per la GPLv2-licenco, kiel rezulto de kiu estis farita decido relicenci ĝin al la Apache License v2. Tamen, ne ĉiuj konsentis ŝanĝi la permesilon de sia kodo, do la programistoj decidis reverki la projekton, rezultigante modernan kodon.

Vi povas legi pli pri la historio de la projekto en la oficiala blogo: "La historio de la projekto FreeRDP".

Uzita kiel ilo por identigi erarojn kaj eblajn vundeblecojn en la kodo. PVS StudioĜi estas statika kodanalizilo por C, C++, C# kaj Java, havebla sur platformoj Windows, Linux и macOS.

La artikolo prezentas nur tiujn erarojn, kiuj ŝajnis al mi plej interesaj.

Memorfluo

V773 La funkcio estis forigita sen liberigi la 'cwd'-montrilon. Memorfuĝo eblas. medio.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;
  ....
}

Ĉi tiu fragmento estis prenita el la subsistemo winpr, kiu efektivigas WINAPI-envolvaĵon por ne-Windows sistemoj, t.e., ĝi estas malpeza analogo de Wine. Ĉi tie vi povas vidi likon: memoro asignita de la funkcio getcwd, estas liberigita nur dum pritraktado de specialaj kazoj. Por ripari la eraron, vi devas aldoni alvokon liberaj после memcpy.

Array ekster limoj

V557 Array transpaso eblas. La valoro de 'event->EventHandlerCount' indekso povus atingi 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++;
  }
  ....
}

Ĉi tiu ekzemplo aldonas novan elementon al la listo eĉ se la nombro da elementoj atingis la maksimumon. Ĉi tie sufiĉas anstataŭigi la funkciigiston <= sur <, por ne iri preter la limoj de la tabelo.

Alia eraro de ĉi tiu tipo estis trovita:

  • V557 Array transpaso eblas. La valoro de 'iBitmapFormat' indekso povus atingi 8. ordoj.c 2623

Tajperaroj

Fragmento 1

V547 Esprimo '!pipe->In' ĉiam estas malvera. 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;
  ....
}

Ĉi tie ni vidas oftan tajperarojn: la dua kondiĉo kontrolas la saman variablon kiel la unua. Plej verŝajne, la eraro aperis kiel rezulto de malsukcesa kopiado de kodo.

Fragmento 2

V760 Du blokoj da identa teksto estis trovitaj. La dua bloko komenciĝas de linio 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);
  ....
}

Alia tajperaro: la kodokomento implicas, ke la fadeno devus veni minorVersion, tamen, legado okazas en variablo nomita majorVersion. Tamen mi ne konas la protokolon, do ĉi tio estas nur supozo.

Fragmento 3

V524 Estas strange ke la korpo de 'trio_index_last' funkcio estas plene ekvivalenta al la korpo de 'trio_index' funkcio. 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);
}

Juĝante laŭ la komento, la funkcio trio_indekso trovas la unuan signan kongruon en ĉeno kiam trio_index_last - lasta afero. Sed la korpoj de ĉi tiuj funkcioj estas identaj! Plej verŝajne ĉi tio estas tajperaro, kaj en la funkcio trio_index_last bezonas uzi strhrchr anstataŭ strchr. Tiam la konduto estos atendata.

Fragmento 4

V769 La "datuma" montrilo en la esprimo egalas nullptr. La rezulta valoro de aritmetikaj operacioj sur ĉi tiu montrilo estas sensenca kaj ĝi ne devus esti uzata. 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;
  ....
}

Ŝajnas, ke ĉi tie hazarde mankis la neiga operatoro ! Proksime datumoj. Estas strange, ke tio pasis nerimarkite.

Fragmento 5

V517 La uzo de 'se (A) {...} else if (A) {...}' ŝablono estis detektita. Estas probableco de logika erarĉeesto. Kontrolu liniojn: 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 <= 0x3FFFFF)
  {
    byte = (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 <= 0x3FFFFF)
  {
    byte = (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);
  }
  ....
}

La lastaj du kondiĉoj estas la samaj: ŝajne iu forgesis kontroli ilin post kopiado. Oni rimarkas el la kodo, ke la lasta parto funkcias kun kvar-bajtaj valoroj, do ni povas supozi, ke la lasta kondiĉo estu valoro <= 0x3FFFFFFFF.

Alia eraro de ĉi tiu tipo estis trovita:

  • V517 La uzo de 'se (A) {...} else if (A) {...}' ŝablono estis detektita. Estas probableco de logika erarĉeesto. Kontrolu liniojn: 169, 173. file.c 169

Valido de enigo-datumoj

Fragmento 1

V547 Esprimo 'strcat(celo, fonto) != NULL' ĉiam estas vera. 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);
}

Kontroli la rezulton de la funkcio en ĉi tiu ekzemplo estas malĝusta. Funkcio strcat resendas montrilon al la fina versio de la ĉeno, t.e. la unua parametro pasis. En ĉi tiu kazo estas celo. Tamen, se ĝi estas egala NULL, tiam estas tro malfrue por kontroli ĝin, ĉar en la funkcio strcat ĝi estos dereferencata.

Fragmento 2

V547 Esprimo 'kaŝmemoro' ĉiam estas vera. glifo.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)
  {
    ....
  }
  ....
}

En ĉi tiu kazo la variablo kaŝmemoro la adreso de senmova tabelo estas atribuita glyphCache->glyphCache. Tiel, kontrolu se (kaŝmemoro) povas esti preterlasita.

Eraro pri administrado de rimedoj

V1005 La rimedo estis akirita per funkcio 'CreateFileA' sed estis liberigita per nekongrua 'fclose' funkcio. atestilo.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;
  }
  ....
}

Dosiera priskribilo fp, kreita per funkciovoko Krei dosieron fermita erare per funkcio fclose de la norma biblioteko, ne CloseHandle.

Samaj kondiĉoj

V581 La kondiĉaj esprimoj de la "se" deklaroj situantaj unu apud la alia estas identaj. Kontrolu liniojn: 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);
  }
  ....
}

Ĉi tiu ekzemplo eble ne estas cimo. Tamen, ambaŭ kondiĉoj enhavas la samajn mesaĝojn, unu el kiuj plej verŝajne povas esti forigita.

Purigado de nulaj montriloj

V575 La nula montrilo estas transdonita al 'libera' funkcio. Inspektu la unuan argumenton. 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);
  ....
}

Funkcii liberaj vi povas pasi nulan montrilon kaj la analizilo scias pri ĝi. Sed se detektas situacion, en kiu la montrilo ĉiam estas nula, kiel en ĉi tiu fragmento, averto estos eligita.

Montrilo mszGroupsA komence egalaj NULL kaj ne estas pravigita ie ajn. La nura branĉo de kodo kie la montrilo povus esti pravalorigita estas neatingebla.

Estis aliaj mesaĝoj kiel:

  • V575 La nula montrilo estas transdonita al 'libera' funkcio. Inspektu la unuan argumenton. permesilo.c 790
  • V575 La nula montrilo estas transdonita al 'libera' funkcio. Inspektu la unuan argumenton. rdpsnd_alsa.c 575

Plej verŝajne, tiaj forgesitaj variabloj ekestas dum la refaktora procezo kaj simple povas esti forigitaj.

Ebla superfluo

V1028 Ebla superfluo. Konsideru ĵeti operandojn, ne la rezulton. 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));
  ....
}

Alportante la rezulton al longaj ne estas superflua protekto, ĉar la kalkulo mem okazas uzante la tipon int.

Montrilo-dereferenco en inicialigo

V595 La "kunteksto" montrilo estis utiligita antaŭ ol ĝi estis kontrolita kontraŭ nullptr. Kontrolu liniojn: 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;
  ....
}

Jen la montrilo kunteksto estas dereferencita dum komencado - antaŭ ol ĝi estas kontrolita.

Aliaj eraroj de ĉi tiu tipo estis trovitaj:

  • V595 La 'ntlm' montrilo estis utiligita antaŭ ol ĝi estis kontrolita kontraŭ nullptr. Kontrolu liniojn: 236, 255. ntlm.c 236
  • V595 La "kunteksto" montrilo estis utiligita antaŭ ol ĝi estis kontrolita kontraŭ nullptr. Kontrolu liniojn: 1003, 1007. rfx.c 1003
  • V595 La montrilo 'rdpei' estis utiligita antaŭ ol ĝi estis kontrolita kontraŭ nullptr. Kontrolu liniojn: 176, 180. rdpei_main.c 176
  • V595 La montrilo 'gdi' estis utiligita antaŭ ol ĝi estis kontrolita kontraŭ nullptr. Kontrolu liniojn: 121, 123. xf_gfx.c 121

Sensignifa kondiĉo

V547 Esprimo 'rdp->state >= CONNECTION_STATE_ACTIVE' ĉiam estas vera. konekto.c 1489

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

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

Estas facile vidi, ke la unua kondiĉo estas sensignifa pro la responda valoro asignita pli frue.

Malĝusta ĉen-analizo

V576 Malĝusta formato. Konsideru kontroli la trian realan argumenton de la funkcio 'sscanf'. Montrilo al la sensigna int-tipo estas atendata. prokurilo.c 220

V560 Parto de kondiĉa esprimo ĉiam estas vera: (rc >= 0). prokurilo.c 222

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

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

La analizilo tuj eligas 2 avertojn por ĉi tiu fragmento. Specifer %u atendas variablon de tipo sennoma int, sed varia sub havas tipon int. Poste ni vidas suspektindan kontrolon: la kondiĉo dekstre ne havas sencon, ĉar komence estas komparo kun unu. Mi ne scias, kion volis diri la aŭtoro de ĉi tiu kodo, sed io klare malĝustas ĉi tie.

Neordaj ĉekoj

V547 Esprimo 'stato == 0x00090314' ĉiam estas malvera. 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;
  ....
}

La kontrolitaj kondiĉoj ĉiam estos malveraj, ĉar ekzekuto nur atingos la duan kondiĉon se stato == SEC_E_OK. La ĝusta kodo povus aspekti jene:

if (status == SEC_I_COMPLETE_NEEDED)
  status = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
  status = SEC_I_CONTINUE_NEEDED;
else if (status != SEC_E_OK)
{
  ....
  return FALSE;
}

konkludo

Tiel, kontroli la projekton malkaŝis multajn problemojn, sed nur la plej interesa parto de ili estis priskribita en la artikolo. Projektaj programistoj povas kontroli la projekton mem petante provizoran permesilon ŝlosilon en la retejo PVS Studio. Estis ankaŭ falsaj pozitivoj, laboro pri kiu helpos plibonigi la analizilon. Tamen, statika analizo estas grava se vi volas ne nur plibonigi la kvaliton de via kodo, sed ankaŭ redukti la tempon pasigitan trovi erarojn, kaj PVS-Studio povas helpi pri tio.

Kontrolante FreeRDP per la analizilo PVS-Studio

Se vi volas dividi ĉi tiun artikolon kun anglalingva publiko, bonvolu uzi la tradukan ligilon: Sergey Larin. Kontrolante FreeRDP kun PVS-Studio

fonto: www.habr.com

Aĉetu fidindan gastigadon por retejoj kun DDoS-protekto, VPS-VDS-serviloj 🔥 Aĉetu fidindan retejan gastigadon kun DDoS-protekto, VPS VDS-servilojn | ProHoster