image1.png

XMage é um aplicativo / servidor de cliente para reproduzir magia : O encontro (MTG). O XMage começou a evoluir no início de 2010. Durante este tempo, 182 lançamentos foram publicados, todo o exército de torcedores atendeu e o projeto ainda se desenvolve ativamente. Uma excelente oportunidade para participar do seu desenvolvimento também! Portanto, hoje, o Unicorn PVS-Studio verificará a base de código XMage, e quem sabe, poderia entrar em conflito com alguém em combate.

Em suma, o projeto

Xmage tem desenvolvido ativamente há 10 anos. Seu objetivo é criar uma versão on-line gratuita e jogo de cartão de código aberto: a coleta original.

  • Acesso a cerca de 19.000 mapas √ļnicos emitidos durante os 20 anos de hist√≥ria MTG;
  • controle autom√°tico e aplica√ß√£o de todas as regras do jogo existente;
  • ;
  • (AI);
  • (padr√£o, moderno, vintage, ordem);
  • ,.

Eu caí no trabalho dos alunos da Universidade de Tecnologia de Delft (Mestrado em Arquitetura de Software). Isso consistiu que os caras deram uma parte ativa em projetos de código aberto, que tiveram que ser bastante complexos e desenvolver ativamente. Ao longo de um período de oito semanas, os alunos estudaram os projetos de curso e abertos para entender e descrever a arquitetura do software selecionado.
ent√£o √© tudo. Neste trabalho, os caras analisaram o projeto XMage, e um dos aspectos de seu trabalho era obter v√°rias m√©tricas usando Sonarqube (n√ļmero de linhas de c√≥digo, complexidade ciclom√°tica, duplica√ß√£o de c√≥digo, odor, erros, vulnerabilidades, etc.).
Minha atenção foi atraída pelo fato de que na época de 2018, a análise do Sonarqube mostrou 700 defeitos (bugs, vulnerabilidades) para 1000000 linhas de código.
Depois de pesquisar na hist√≥ria dos contribuintes, descobri que do relat√≥rio recebido com avisos, eles fizeram uma solicita√ß√£o de tra√ß√£o para corrigir cerca de 30 defeitos na categoria “Bloqueador” ou “Cr√≠tico”. E o resto dos avisos n√£o √© conhecido, mas espero que eles n√£o tenham sido perdidos.
Foi 2 anos desde então e a base de código aumentou em cerca de 250.000 linhas de código Рuma boa razão para ver como as coisas estão acontecendo.

Sobre a an√°lise

para an√°lise, peguei a vers√£o XMage – 1.4.44v0.
Eu tinha muita sorte com o projeto. O edifício XMation com o Maven ficou muito simples (como foi escrito na documentação):

mvn clean install -DskipTests

nada mais foi necess√°rio de mim. Legal?
A integração do plugin PVS-Studio no Maven não tem nenhum problema: tudo é como na documentação. Após a análise, 911 avisos foram recebidos, incluindo 674 para advertências de 1 e 2 níveis de confiança. Para os fins deste artigo, não tomei em conta os avisos de nível 3 porque geralmente há uma alta porcentagem de falsos positivos. Eu gostaria de chamar sua atenção para o fato de que quando você usa um analisador estático em uma verdadeira batalha, você não pode ignorar tais avisos porque também podem indicar defeitos significativos no código. Além disso, eu não levei em conta os avisos de certas regras, com base em que eles são melhor levados em conta por aqueles que conhecem o projeto como eu:

  • v6022, /. 336.
  • v6014 ,,,, 73.
  • v6021 ,,. 36.
  • v6048 ,,. 17.

Além disso, várias regras de diagnóstico produziram cerca de 20 positivos óbvios falsos do mesmo tipo. Salvo em TODO!
Consequentemente, se subtraíamos tudo, cerca de 190 positivos foram submetidos a consideração. Ao considerar os gatilhos, muitos defeitos menores do mesmo tipo foram identificados, que estavam relacionados à depuração ou de uma operação de verificação ou sem sentido. Além disso, muitos positivos foram associados a um pedaço muito estranho de código solicitado a ser rejeitado.
Consequentemente, para este artigo, identifiquei 11 regras de diagnóstico e analisei um dos gatilhos mais interessantes.
Vamos dar uma olhada no que aconteceu.

N1 Aviso

V6003 O uso do modelo ‘IF (cart√£o! = null) {…} else if (cart√£o! = null) {…}’ foi detectou. H√° uma probabilidade de presen√ßa de erro l√≥gico. Torrentialgearhulk.Java (90), torrentialgearhulk.java (102)

@Overridepublic boolean apply(Game game, Ability source) { .... Card card = game.getCard(....); if (card != null) { .... } else if (card != null) { .... } ....}

tudo é simples aqui: o corpo da segunda instrução condicional se (cartão! = Nulo) no If-else Рse a construção nunca será executada, já que o programa não chegará a este ponto ou cartão! = NULL sempre estará errado.

N2 Aviso

V6004 A instru√ß√£o “ent√£o” √© equivalente √† instru√ß√£o “else”. AshoughEffectimpl.java (35), AshoughEffectimpl.java (37)

@Overridepublic boolean applies(....) { // affectedControllerId = player to check if (getAsThoughEffectType().equals(AsThoughEffectType.LOOK_AT_FACE_DOWN)) { return applies(objectId, source, playerId, game); } else { return applies(objectId, source, playerId, game); }}

um erro comum que ocorreu na minha pr√°tica de verifica√ß√£o de projetos de c√≥digo aberto. Copiar colar? Ou eu sinto falta de alguma coisa? Eu acho que voc√™ ainda tem que retornar falso para o ramo. PS Se alguma coisa, n√£o h√° nenhuma chamada recursiva se aplica (….), porque s√£o m√©todos diferentes. Gatilho similar:

  • v6004 A instru√ß√£o “ent√£o” √© equivalente √† declara√ß√£o “else”. GUIDISPLAYUTIL.JAVA (194), GUIDISPLAYUTIL.JAVA (198)
  • aviso N3

    v6007 express√£o ‘filtro.getmessage (). TOLOWCASE (local.English) .Startswith (“Cada”) est√° sempre errado. SetPowerThnessaffect.java (107)

    @Overridepublic String getText(Mode mode) { StringBuilder sb = new StringBuilder(); .... if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) { sb.append(" has base power and toughness "); } else { sb.append(" have base power and toughness "); } .... return sb.toString();}

    Regras de diagnóstico Os triggers V6007 são muito populares para cada projeto ser verificado. XMage não é exceção (79 peças). Atuação da regra, em princípio, tudo está no caso, mas muitos casos caem na depuração, depois em resseguro, então em outra coisa. Em geral, é melhor que o autor do código monitore tais positivos do que para mim.
    Este gatilho, no entanto, √© definitivamente um erro. Dependendo do in√≠cio do filtro.GetMessage Line () para o texto seccionado “a …” ou “ter …” √© adicionado. Mas o erro √© que os desenvolvedores confirmam que a corrente come√ßa com uma letra mai√ļscula, depois de converter a mesma corrente em min√ļsculas antes disso. Oops. Como resultado, a linha adicionada ser√° sempre “ter …”. O resultado da falha n√£o √© cr√≠tico, mas tamb√©m desagrad√°vel: um texto composto analfabeto aparecer√° em algum lugar. Os pontos positivos que encontrei o mais interessante:

    • v6007 A express√£o ‘t.startswith (“-“) “√© sempre falsa. Boostsourceeffect.java (103)
    • a express√£o v6007 ‘setnames.isempty () √© sempre falso. DownloadPicturesservice.java (300)
    • a express√£o v6007 ‘existentebucketName == null’ est√° sempre errado. S3uploader.java (23)
    • v6007 express√£o ‘! Lastrule.endswith (“.”) √Č sempre verdade. Efeitos.java (76)
    • a express√£o v6007 ‘subtipopestoignore :: cont√©m’ √© sempre falso. Verifycarddatatest.java (893)
    • a express√£o v6007 ‘notstartedtables == 1’ √© sempre falso. Mageserverimpl.java (1330)

    n4 aviso

    v6008 Dereference null de “savedspecialares”. Dragonsmaze.java (230)

    public final class DragonsMaze extends ExpansionSet { .... private List<CardInfo> savedSpecialRares = new ArrayList<>(); .... @Override public List<CardInfo> getSpecialRare() { if (savedSpecialRares == null) { // <= CardCriteria criteria = new CardCriteria(); criteria.setCodes("GTC").name("Breeding Pool"); savedSpecialRares.addAll(....); // <= criteria = new CardCriteria(); criteria.setCodes("GTC").name("Godless Shrine"); savedSpecialRares.addAll(....); .... } return new ArrayList<>(savedSpecialRares); }}

    O analisador Reclama√ß√Ķes do deserreferenciamento da refer√™ncia NULL SavedSpecialares Quando a execu√ß√£o atinge o primeiro enchimento da coleta.
    A primeira coisa que vem à minha mente é simplesmente confundir Savepeciallares == Null com SavedSpecialares! = Nulo. Mas nesse caso, o NPE pode ocorrer no construtor de arraylist quando a coleção é retornada pelo método, porque Savepecialares == null é sempre possível. Corrija o código com a primeira solução que vem à minha mente não é uma boa opção. Depois de um pouco de compreensão do código, descobri que o ADSpecialares é imediatamente definido por uma coleção vazia quando é declarada e não é realocada em qualquer outro lugar. Isso nos diz quantas perguntas nunca serão zero, e o desreferenciamento de uma referência zero, cujo analisador avisa, nunca acontecerá, porque nunca alcançará a coleção. Como resultado, o método sempre retornará uma coleção vazia.

    PS Para resolver esse problema, você deve substituir SavedSpecialares == NULL por SavedSpecialrares.isempty ().
    PPS alas, jogando em xmage, voc√™ n√£o ser√° capaz de obter cart√Ķes raros especiais para a cole√ß√£o do labirinto do drag√£o.
    Outro caso de desrespeito de uma referência zero:

    • v6008 zero desert√™ncia de “correspond√™ncia”. TableController.java (973)

    aviso n5

    v6012 operador ‘?:’, Seja qual for a sua express√£o condicional, sempre retorna apenas um e o mesmo valor ‘table.GetTime ( ) ‘. Tabanager.java (418), TableManager.java (418)

    private void checkTableHealthState() { .... logger.debug(.... + formatter.format(table.getStartTime() == null ? table.getCreateTime() : table.getCreateTime()) + ....); ....}

    Aqui o operador ternário?: Retorna o mesmo valor, independentemente da tabela.getStartTime Condition () == null. Eu acho que a conclusão do código jogou uma piada cruel para o desenvolvedor. Opção de correção:

    private void checkTableHealthState() { .... logger.debug(.... + formatter.format(table.getStartTime() == null ? table.getCreateTime() : table.getStartTime()) + ....); ....}

    n6

    aviso
    v6026 Este valor j√° est√° atribu√≠do √† vari√°vel “this.loseout .TornarCreateTyPethargetEfect.java (54)

    publicBecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) { super(effect); this.subtypes.addAll(effect.subtypes); this.loseOther = effect.loseOther; this.loseOther = effect.loseOther;}

    cadeia de atribui√ß√£o dupla. Parece que o desenvolvedor foi um pouco empolgado com as teclas de atalho e n√£o notificou. Mas desde que o efeito tem um grande n√ļmero de √°reas, o fragmento merece ser focado.

    N7 Aviso

    V6036 O valor da op√ß√£o ‘Selects’ n√£o inicializa √© usado. Session.java (227)

    public String connectUserHandling(String userName, String password){ .... if (!selectUser.isPresent()) { // user already exists selectUser = UserManager.instance.getUserByName(userName); if (selectUser.isPresent()) { User user = selectUser.get(); .... } } User user = selectUser.get(); // <= ....}

    A partir do analisador de aviso, podemos concluir que a seleção.Get () pode lançar um Nosuchelement.
    Vamos olhar mais de perto o que est√° acontecendo aqui.
    Se você acha que o comentário que o usuário já existe, nenhuma exceção será levantada:

    ....if (!selectUser.isPresent()) { // user already exists ....}User user = selectUser.get()....

    No presente caso, a execu√ß√£o do programa N ‘n√£o entra no corpo de instru√ß√£o condicional. E tudo ficar√° bem. Mas ent√£o a quest√£o surge: por que precisamos de um operador condicional com uma esp√©cie de l√≥gica complexa se nunca √© executada?

    Mas o que acontece se o comentário não é nada?

    ....if (!selectUser.isPresent()) { // user already exists selectUser = UserManager.instance.getUserByName(userName); if (selectUser.isPresent()) { .... }}User user = selectUser.get(); // <=....

    então, a execução entra no corpo da instrução condicional e recupera o usuário via GeruserbyName (). A validade do usuário é novamente verificada, sugerindo que a seleção não pode ser inicializada. Não há outro ramo para este caso, que também levará a um NosuchelementException na linha de código em questão.

    N8 Aviso

    V6042 A compatibilidade de express√£o com o tipo “A” √© verificada, mas √© convertida no tipo “B”. Caixa de sele√ß√£o.java (586)

    /** * sets the model - must be an instance of CheckBoxListModel * * @param model the model to use * @throws IllegalArgumentException if the model is not an instance of * CheckBoxListModel * @see CheckBoxListModel */@Overridepublic void setModel(ListModel model) { if (!(model instanceof CheckBoxListModel)) { if (model instanceof javax.swing.DefaultListModel) { super.setModel((CheckBoxListModel)model); // <= } else { throw new IllegalArgumentException( "Model must be an instance of CheckBoxListModel!"); } } else { super.setModel(model); }}

    o autor do c√≥digo √© confuso sobre algo aqui: primeiro ele garante que o modelo n ‘n√£o √© um caixa de sele√ß√£o, ent√£o , converte explicitamente o objeto nesse tipo. Devido a isso, o m√©todo SetModel lan√ßar√° imediatamente um classcastException quando chegar l√°.
    O arquivo caixa de sele√ß√£o foi adicionado h√° 2 anos e este bug persiste no c√≥digo desde ent√£o. Aparentemente, n√£o h√° testes para configura√ß√Ķes incorretas, n√£o h√° uso real deste m√©todo com tipos inadequados de tipos, ent√£o vive.
    Se de repente, algu√©m se conecta a este m√©todo e l√™ Javadoc, ele espera uma ilegalargumentException, n√£o um classcastException … Eu n√£o acho que algu√©m deliberadamente coloque com essa exce√ß√£o, mas quem sabe.
    Dada a documentação, o código provavelmente parece ser este:

    public void setModel(ListModel model) { if (!(model instanceof CheckBoxListModel)) { throw new IllegalArgumentException( "Model must be an instance of CheckBoxListModel!"); } else { super.setModel(model); }}

    aviso N9

    V6060 Refer√™ncia ‘player ‘foi usado antes de ser verificado contra nulo. VIGEANINTUITION.JAVA (79), VigeanInTuition.java (78)

    v6060 adverte o desenvolvedor Um objeto está sendo acesso antes que ele não seja verificado para NULL. Os gatilhos desta regra são frequentemente encontrados nos artigos sobre auditoria de projeto de código aberto: Geralmente, o motivo é o fracasso da refatoração ou a modificação de contratos para métodos. Se você prestar atenção ao método GetPlayer (), tudo será imediatamente configurado:

    // Result must be checked for null.// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)Player getPlayer(UUID playerId);

    n10 aviso

    v6072 Dois fragmentos de c√≥digo semelhantes foi encontrado. Isso pode ser uma falha de digita√ß√£o e a vari√°vel “playerb” deve ser usada em vez de “Playera”. Subtipopechingeffectstest.java (162), subtipopechangingefectstest.java (158), subtipopechangingefectstest.java (156), subtipopechangingefectstest.java (160)

    @Testpublic void testArcaneAdaptationGiveType() { addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); // Enchantment {2}{U} addCard(Zone.BATTLEFIELD, playerA, "Island", 3); addCard(Zone.HAND, playerA, "Silvercoat Lion"); addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion"); addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <= addCard(Zone.HAND, playerB, "Silvercoat Lion"); addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion"); addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <= .... for (Card card : playerB.getGraveyard().getCards(currentGame)) { if (card.isCreature()) { Assert.assertEquals(card.getName() + " should not have ORC type", false, card.getSubtype(currentGame).contains(SubType.ORC)); Assert.assertEquals(card.getName() + " should have CAT type", true, card.getSubtype(currentGame).contains(SubType.CAT)); } }}

    tendo visto que este erro est√° no Testes, voc√™ pode imediatamente desvalorizar a falha encontrada em pensar: “Bem, estes s√£o testes”. Se este √© o caso, eu n√£o concordo com voc√™. Afinal, os testes desempenham um papel bastante importante no desenvolvimento (mas n√£o t√£o percept√≠vel quanto a programa√ß√£o), e quando uma falha aparece em uma vers√£o, eles imediatamente come√ßam a apontar os testes / testadores. Assim, os testes defeituosos s√£o insustent√°veis. Por que assim s√£o necess√°rios esses testes? Por que os recursos desperdi√ßados neles?
    O m√©todo TestarCaneadAptyGiveType () Testa o cart√£o “Adapta√ß√£o Arcana”. Cada jogador recebe cart√Ķes em uma √°rea de jogos espec√≠fica. E gra√ßas √† Copy-Pasta, Playera tem 2 cart√Ķes “Silvercoat Lion” id√™nticos no playground “Cemit√©rio”, e o playerBdun nada aconteceu. Al√©m disso, uma esp√©cie de magia e tente testar.
    Quando o teste chegar ao “cemit√©rio” do playerb no rally atual, ent√£o a execu√ß√£o do teste nunca entra no loop, porque n√£o havia nada no “cemit√©rio”. Isto √© o que eu descobri com o bom antigo sistema.out.println () ao iniciar o teste.
    Copy-paste corrigido:

    ....addCard(Zone.HAND, playerA, "Silvercoat Lion");addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=addCard(Zone.HAND, playerB, "Silvercoat Lion");addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");addCard(Zone.GRAVEYARD, playerB, "Silvercoat Lion"); // <=....

    Ap√≥s alterar o c√≥digo, quando eu executei o teste, a busca por criaturas no cemit√©rio do playerb A come√ßou a funcionar. Ave, system.out.println ()! O teste √© verde antes e depois da corre√ß√£o, o que √© muita sorte. Mas em caso de modifica√ß√Ķes que modificam a l√≥gica da Execu√ß√£o do Programa, tal teste lhe far√° mau servi√ßo, informando o sucesso mesmo se houver erros.
    mesmo cópia em outros lugares:

    • v6072 Dois fragmentos de c√≥digo semelhantes foram encontrados. Isso pode ser uma falha de digita√ß√£o e a vari√°vel “playerb” deve ser usada em vez de “Playera”. PaintersServanttest.java (33), PaintersServanttest.java (29), PaintersServanttest.java (27), PaintersServanttest.java (31)
    • v6072 Dois fragmentos de c√≥digo semelhantes foram encontrados. Isso pode ser uma falha de digita√ß√£o e a vari√°vel “playerb” deve ser usada em vez de “Playera”. Subtipopechingeffectstest.java (32), subtipopechangingeffectstest.java (28), subtiloechangingefectstest.java (26), subtiloechangingefectstest.java (30)

    aviso N11

    v6086 c√≥digo suspeito formata√ß√£o. A palavra-chave “else” provavelmente est√° faltando. Deckimporter.java (23)

    public static DeckImporter getDeckImporter(String file) { if (file == null) { return null; } if (file.toLowerCase(Locale.ENGLISH).endsWith("dec")) { // <= return new DecDeckImporter(); } else if (file.toLowerCase(Locale.ENGLISH).endsWith("mwdeck")) { return new MWSDeckImporter(); } else if (file.toLowerCase(Locale.ENGLISH).endsWith("txt")) { return new TxtDeckImporter(haveSideboardSection(file)); } .... else { return null; }}

    a regra de diagnóstico V6086 Diagnóstico Um incorreto se Рse formatando, envolvendo a omissão de outra pessoa.
    Este extrato de código ilustra isso. Neste caso, por causa da expressão do retorno nulo, uma imprecisão na formatação não leva a nada, mas é legal encontrar esses casos, porque não é necessário. Considere um caso em que a omissão do outro pode levar a um comportamento inesperado: No caso de Obj = nulo, o objeto em questão será atribuído Um valor, e a falta mais fará com que o objeto recém-designado verifique ao longo da cadeia IF-else, enquanto o objeto deveria retornar imediatamente ao método.

    Conclus√£o

    Verifica√ß√£o de xmage √© outro artigo que revela as capacidades de modernos analisadores est√°ticos. No desenvolvimento moderno, sua necessidade s√≥ aumenta, j√° que a complexidade do software aumenta. E independentemente do n√ļmero de vers√Ķes, testes, coment√°rios do usu√°rio: Um bug sempre encontrar√° uma falha para inserir sua base de c√≥digo. Ent√£o, por que n√£o adicionar outra barreira √† sua defesa?
    Como você entende, os analisadores estão sujeitos a falsos positivos (incluindo o PVS-Studio Java). Este pode ser o resultado de uma falha óbvia e um código muito confuso (ALAS, o analisador não entendia). Você tem que tratá-los com compreensão e cancelar a inscrição imediatamente sem hesitação, mas enquanto falsos positivos estão esperando por sua correção, você pode usar um dos métodos de advertência.
    Conclus√£o, eu pessoalmente sugiro que voc√™ “toque” o analisador baixando-o do nosso site.

    se voc√™ quiser compartilhar este artigo com um p√ļblico que fala ingl√™s, por favor use o Link de tradu√ß√£o: Maxim Stefanov. Verificando o c√≥digo XMage e por que voc√™ n√£o poder√° obter os cart√Ķes raros especiais da cole√ß√£o do labirinto do drag√£o.

    Leave a comment

    O seu endereço de email não será publicado. Campos obrigatórios marcados com *