Au cours des dix dernières années, le mouvement open-source a été l'un des principaux moteurs du développement de l'industrie informatique et sa composante cruciale. Le rôle des projets open source prend de plus en plus d'importance non seulement en termes de quantité mais également en termes de qualité, ce qui modifie le concept même de leur positionnement sur le marché informatique en général. Notre courageuse équipe PVS-Studio ne reste pas les bras croisés et participe activement au renforcement de la présence de logiciels open source en trouvant des bogues cachés dans les profondeurs énormes des bases de code et en offrant des options de licence gratuites aux auteurs de tels projets. Cet article n'est qu'une autre partie de cette activité! Aujourd'hui, nous allons parler d'Apache Hive. J'ai le rapport - et il y a des choses qui méritent d'être examinées.
À propos de PVS-Studio
L'analyseur de code statique
PVS-Studio , qui existe depuis plus de 10 ans maintenant, est une solution logicielle multifonctionnelle et facile à intégrer. À l'heure actuelle, il prend en charge C, C ++, C # et Java et s'exécute sur Windows, Linux et macOS.
PVS-Studio est une solution B2B payante utilisée par de nombreuses équipes dans plusieurs entreprises. Si vous voulez essayer l'analyseur, visitez cette
page pour télécharger la distribution et demander une clé d'essai.
Si vous êtes un geek open-source ou, disons, un étudiant, vous pouvez profiter de l'une de nos
options de licence gratuites.
À propos d'Apache Hive
La quantité de données a augmenté à un rythme énorme au cours des dernières années. Les bases de données standard ne peuvent plus faire face à cette croissance rapide, d'où le terme Big Data ainsi que d'autres notions connexes (telles que le traitement, le stockage et d'autres opérations sur les Big Data).
Apache Hadoop est actuellement considéré comme l'une des technologies pionnières du Big Data. Ses tâches principales sont le stockage, le traitement et la gestion de grandes quantités de données. Les principaux composants du cadre sont Hadoop Common,
HDFS ,
Hadoop MapReduce et
Hadoop YARN . Au fil du temps, un vaste écosystème de projets et de technologies connexes s'est développé autour de Hadoop, dont beaucoup ont initialement commencé dans le cadre du projet, puis se sont développés pour devenir indépendants.
Apache Hive est l'un d'entre eux.
Apache Hive est un entrepôt de données distribué. Il gère les données stockées dans HDFS et fournit le langage de requête basé sur SQL (HiveQL) pour gérer ces données. Plus de détails sur le projet peuvent être trouvés
ici .
Exécution de l'analyse
Il n'a pas fallu beaucoup d'efforts ou de temps pour commencer l'analyse. Voici mon algorithme:
- Téléchargé Apache Hive depuis GitHub ;
- Lisez le guide sur le démarrage de l'analyseur Java et lancez l'analyse;
- A obtenu le rapport de l'analyseur, l'a étudié et a écrit les cas les plus intéressants.
Les résultats de l'analyse sont les suivants: 1456 avertissements de niveaux élevé et moyen (602 et 854 respectivement) sur plus de 6 500 fichiers.
Tous les avertissements ne font pas référence à de véritables bogues. C'est tout à fait normal; vous devez modifier les paramètres de l'analyseur avant de commencer à l'utiliser régulièrement. Après cela, vous vous attendez généralement à un taux assez faible de faux positifs (
exemple ).
J'ai omis les 407 avertissements (177 niveau élevé et 230 niveau moyen) déclenchés par les fichiers de test. J'ai également ignoré le diagnostic
V6022 (car vous ne pouvez pas distinguer de manière fiable les fragments défectueux et corrects lorsque vous n'êtes pas familier avec le code), qui a été déclenché jusqu'à 482 fois. Je n'ai pas non plus examiné les 179 avertissements générés par le diagnostic
V6021 .
En fin de compte, j'avais encore assez d'avertissements pour y aller, et comme je n'ai pas modifié les paramètres, il y a toujours un certain pourcentage de faux positifs parmi eux. Cela ne sert à rien d'inclure trop d'avertissements dans un article comme celui-ci :). Nous allons donc simplement parler de ce qui a attiré mon attention et qui avait l'air assez curieux.
Conditions prédéterminées
Parmi les diagnostics examinés pour cette analyse, le
V6007 détient un record pour le nombre d'avertissements émis. Un peu plus de 200 messages !!! Certains semblent inoffensifs, d'autres méfiants et d'autres sont de véritables bogues après tout! Jetons un coup d'œil à certains d'entre eux.
V6007 Expression 'key.startsWith ("hplsql.")' Est toujours vrai. Exec.java (675)
void initOptions() { .... if (key == null || value == null || !key.startsWith("hplsql.")) {
C'est une construction if-else-if assez longue! L'analyseur n'aime pas la condition dans le dernier
if (key.startsWith ("hplsql.")) Parce que si l'exécution l'atteint, cela signifie que c'est vrai. En effet, si vous regardez la première ligne de cette construction if-else-if, vous verrez qu'elle contient déjà la vérification opposée, donc si la chaîne ne commence pas par
"hplsql". , l'exécution sautera immédiatement à la prochaine itération.
V6007 L' expression 'columnNameProperty.length () == 0' est toujours fausse. OrcRecordUpdater.java (238)
private static TypeDescription getTypeDescriptionFromTableProperties(....) { .... if (tableProperties != null) { final String columnNameProperty = ....; final String columnTypeProperty = ....; if ( !Strings.isNullOrEmpty(columnNameProperty) && !Strings.isNullOrEmpty(columnTypeProperty)) { List<String> columnNames = columnNameProperty.length() == 0 ? new ArrayList<String>() : ....; List<TypeInfo> columnTypes = columnTypeProperty.length() == 0 ? new ArrayList<TypeInfo>() : ....; .... } } } .... }
La comparaison de la
longueur de la chaîne
columnNameProperty avec zéro renvoie toujours
false . Cela se produit car cette comparaison suit la
vérification! Strings.isNullOrEmpty (columnNameProperty) . Donc, si l'exécution atteint notre condition, cela signifie que la chaîne
columnNameProperty n'est sûrement ni nulle ni vide.
Il en va de même pour la chaîne
columnTypeProperty une ligne plus tard:
- V6007 L'expression 'columnTypeProperty.length () == 0' est toujours fausse. OrcRecordUpdater.java (239)
V6007 L' expression 'colOrScalar1.equals ("Column")' est toujours fausse. GenVectorCode.java (3469)
private void generateDateTimeArithmeticIntervalYearMonth(String[] tdesc) throws Exception { .... String colOrScalar1 = tdesc[4]; .... String colOrScalar2 = tdesc[6]; .... if (colOrScalar1.equals("Col") && colOrScalar1.equals("Column"))
Le bon vieux copier-coller. Du point de vue de la logique actuelle, la chaîne
colOrScalar1 peut avoir deux valeurs différentes à la fois, ce qui est impossible. De toute évidence, les contrôles doivent avoir la variable
colOrScalar1 à gauche et
colOrScalar2 à droite.
Avertissements similaires quelques lignes ci-dessous:
- V6007 L'expression 'colOrScalar1.equals ("Scalar")' est toujours fausse. GenVectorCode.java (3475)
- V6007 L'expression 'colOrScalar1.equals ("Column")' est toujours fausse. GenVectorCode.java (3486)
Par conséquent, cette construction if-else-if ne fera jamais rien.
Quelques autres avertissements
V6007 :
- V6007 L'expression "caractères == null" est toujours fausse. RandomTypeUtil.java (43)
- V6007 L'expression 'writeIdHwm> 0' est toujours fausse. TxnHandler.java (1603)
- V6007 L'expression 'fields.equals ("*")' est toujours vraie. Server.java (983)
- V6007 L'expression 'currentGroups! = Null' est toujours vraie. GenericUDFCurrentGroups.java (90)
- V6007 L'expression 'this.wh == null' est toujours fausse. Nouveau retourne une référence non nulle. StorageBasedAuthorizationProvider.java (93), StorageBasedAuthorizationProvider.java (92)
- et ainsi de suite ...
NPE
V6008 Déréférence nulle potentielle de 'dagLock'. QueryTracker.java (557), QueryTracker.java (553)
private void handleFragmentCompleteExternalQuery(QueryInfo queryInfo) { if (queryInfo.isExternalQuery()) { ReadWriteLock dagLock = getDagLock(queryInfo.getQueryIdentifier()); if (dagLock == null) { LOG.warn("Ignoring fragment completion for unknown query: {}", queryInfo.getQueryIdentifier()); } boolean locked = dagLock.writeLock().tryLock(); ..... } }
Un objet nul est intercepté, enregistré et ... le programme continue de fonctionner. Par conséquent, la vérification est suivie d'une déréférence de pointeur nulle. Aïe!
Les développeurs doivent avoir réellement voulu que le programme quitte la fonction ou lève une exception spéciale dans le cas de l'obtention d'une référence nulle.
V6008 Déréférence nulle de 'buffer' dans la fonction 'unlockSingleBuffer'. MetadataCache.java (410), MetadataCache.java (465)
private boolean lockBuffer(LlapBufferOrBuffers buffers, ....) { LlapAllocatorBuffer buffer = buffers.getSingleLlapBuffer(); if (buffer != null) {
Un autre NPE potentiel. Si l'exécution atteint la méthode
unlockSingleBuffer , cela signifie que l'objet
tampon est nul. Supposons que c'est ce qui s'est passé! Si vous regardez la méthode
unlockSingleBuffer , vous remarquerez comment notre objet est déréférencé directement dans la première ligne. Gotcha!
Un changement devenu fou
V6034 Le décalage de la valeur de 'bitShiftsInWord - 1' peut être incompatible avec la taille du type: 'bitShiftsInWord - 1' = [-1 ... 30]. UnsignedInt128.java (1791)
private void shiftRightDestructive(int wordShifts, int bitShiftsInWord, boolean roundUp) { if (wordShifts == 0 && bitShiftsInWord == 0) { return; } assert (wordShifts >= 0); assert (bitShiftsInWord >= 0); assert (bitShiftsInWord < 32); if (wordShifts >= 4) { zeroClear(); return; } final int shiftRestore = 32 - bitShiftsInWord;
Il s'agit d'un décalage potentiel de -1. Si la méthode est appelée avec, disons,
wordShifts == 3 et
bitShiftsInWord == 0 , la ligne signalée se terminera par 1 << -1. Est-ce un comportement planifié?
V6034 Le décalage de la valeur de 'j' peut être incompatible avec la taille du type: 'j' = [0 ... 63]. IoTrace.java (272)
public void logSargResult(int stripeIx, boolean[] rgsToRead) { .... for (int i = 0, valOffset = 0; i < elements; ++i, valOffset += 64) { long val = 0; for (int j = 0; j < 64; ++j) { int ix = valOffset + j; if (rgsToRead.length == ix) break; if (!rgsToRead[ix]) continue; val = val | (1 << j);
Dans la ligne rapportée, la variable
j peut avoir une valeur dans la plage [0 ... 63]. Pour cette raison, le calcul de la valeur de
val dans la boucle peut s'exécuter de manière inattendue. Dans l'expression
(1 << j) , la valeur 1 est de type
int , donc la décaler de 32 bits et plus nous emmène au-delà des limites de la plage du type. Cela peut être corrigé en écrivant
((long) 1 << j) .
Se laisser emporter par la journalisation
V6046 Format incorrect. Un nombre différent d'éléments de format est attendu. Arguments non utilisés: 1, 2. StatsSources.java (89)
private static ImmutableList<PersistedRuntimeStats> extractStatsFromPlanMapper (....) { .... if (stat.size() > 1 || sig.size() > 1) { StringBuffer sb = new StringBuffer(); sb.append(String.format( "expected(stat-sig) 1-1, got {}-{} ;",
Lors de l'écriture du code pour formater la chaîne à l'aide de
String.format () , le développeur a utilisé une syntaxe incorrecte. Par conséquent, les paramètres passés n'ont jamais atteint la chaîne résultante. Je suppose que le développeur avait travaillé sur la journalisation avant d'écrire ceci, d'où il a emprunté la syntaxe.
Une exception volée
V6051 L'utilisation de l'instruction 'return' dans le bloc 'finalement' peut entraîner la perte d'exceptions non gérées. ObjectStore.java (9080)
private List<MPartitionColumnStatistics> getMPartitionColumnStatistics(....) throws NoSuchObjectException, MetaException { boolean committed = false; try { .... committed = commitTransaction(); return result; } catch (Exception ex) { LOG.error("Error retrieving statistics via jdo", ex); if (ex instanceof MetaException) { throw (MetaException) ex; } throw new MetaException(ex.getMessage()); } finally { if (!committed) { rollbackTransaction(); return Lists.newArrayList(); } } }
Renvoyer quoi que ce soit du bloc
finally est une très mauvaise pratique, et cet exemple montre clairement pourquoi.
Dans le bloc
try , le programme forme une demande et accède au stockage. La variable validée a la valeur
false par défaut et ne change son état qu'après l'exécution réussie de toutes les actions précédentes du bloc
try . Cela signifie que si une exception est déclenchée, cette variable sera toujours
fausse . Le bloc catch interceptera l'exception, l'ajustera un peu et le lancera. Donc, quand c'est le tour du bloc
finalement , l'exécution entrera dans la condition à partir de laquelle une liste vide sera retournée. Que nous coûte ce retour? Eh bien, cela nous coûte d'éviter que toute exception capturée ne soit jetée à l'extérieur où elle pourrait être correctement gérée. Aucune des exceptions spécifiées dans la signature de la méthode ne sera levée; ils sont tout simplement trompeurs.
Un message de diagnostic similaire:
- V6051 L'utilisation de l'instruction 'return' dans le bloc 'finalement' peut entraîner la perte d'exceptions non gérées. ObjectStore.java (808)
Divers
V6009 La fonction 'compareTo' reçoit un argument impair. Un objet 'o2.getWorkerIdentity ()' est utilisé comme argument de sa propre méthode. LlapFixedRegistryImpl.java (244)
@Override public List<LlapServiceInstance> getAllInstancesOrdered(....) { .... Collections.sort(list, new Comparator<LlapServiceInstance>() { @Override public int compare(LlapServiceInstance o1, LlapServiceInstance o2) { return o2.getWorkerIdentity().compareTo(o2.getWorkerIdentity());
Il pourrait y avoir un certain nombre de causes menant à une erreur aussi stupide: copier-coller, négligence, hâte, etc. Nous voyons souvent des erreurs comme ça dans les projets open-source et nous avons même un
article entier à ce sujet.
V6020 Divisez par zéro. La plage des valeurs du dénominateur du «diviseur» comprend zéro. SqlMathUtil.java (265)
public static long divideUnsignedLong(long dividend, long divisor) { if (divisor < 0L) { return (compareUnsignedLong(dividend, divisor)) < 0 ? 0L : 1L; } if (dividend >= 0) {
Celui-ci est assez banal. Une série de contrôles a été impuissante pour éviter la division par zéro.
Quelques avertissements supplémentaires:
- V6020 Mod par zéro. La plage des valeurs du dénominateur du «diviseur» comprend zéro. SqlMathUtil.java (309)
- V6020 Divisez par zéro. La plage des valeurs du dénominateur du «diviseur» comprend zéro. SqlMathUtil.java (276)
- V6020 Divisez par zéro. La plage des valeurs du dénominateur du «diviseur» comprend zéro. SqlMathUtil.java (312)
V6030 La méthode située à droite du '|' L'opérateur sera appelé quelle que soit la valeur de l'opérande gauche. Peut-être vaut-il mieux utiliser '||'. OperatorUtils.java (573)
public static Operator<? extends OperatorDesc> findSourceRS(....) { .... List<Operator<? extends OperatorDesc>> parents = ....; if (parents == null | parents.isEmpty()) {
Le programmeur a écrit l'opérateur au niveau du bit | au lieu de la logique ||. Cela signifie que la partie droite sera exécutée quel que soit le résultat de la partie gauche. Si
parents == null , cette faute de frappe se terminera avec un droit NPE dans la prochaine sous-expression logique.
V6042 La
compatibilité de l'expression avec le type «A» est vérifiée, mais elle est convertie en type «B». VectorColumnAssignFactory.java (347)
public static VectorColumnAssign buildObjectAssign(VectorizedRowBatch outputBatch, int outColIndex, PrimitiveCategory category) throws HiveException { VectorColumnAssign outVCA = null; ColumnVector destCol = outputBatch.cols[outColIndex]; if (destCol == null) { .... } else if (destCol instanceof LongColumnVector) { switch(category) { .... case LONG: outVCA = new VectorLongColumnAssign() { .... } .init(.... , (LongColumnVector) destCol); break; case TIMESTAMP: outVCA = new VectorTimestampColumnAssign() { .... }.init(...., (TimestampColumnVector) destCol);
Nous sommes intéressés par les classes
LongColumnVector étend ColumnVector et
TimestampColumnVector étend ColumnVector . La vérification que l'objet
destCol est une instance de
LongColumnVector suggère explicitement que c'est un objet de cette classe qui sera traité dans le corps de l'instruction conditionnelle. Malgré cela, cependant, il est toujours casté dans
TimestampColumnVector ! Comme vous pouvez le voir, ce sont des classes différentes, sauf qu'elles sont dérivées du même parent. Par conséquent, nous obtenons une
exception ClassCastException .
La même chose est vraie dans le cas de la conversion en
IntervalDayTimeColumnVector :
- V6042 La compatibilité de l'expression avec le type «A» est vérifiée, mais elle est convertie en type «B». VectorColumnAssignFactory.java (390)
V6060 La référence «var» a été utilisée avant d'être vérifiée par rapport à null. Var.java (402), Var.java (395)
@Override public boolean equals(Object obj) { if (getClass() != obj.getClass()) {
Ici, vous voyez une étrange vérification de l'objet
var pour
null après que la déréférence s'est déjà produite. Dans ce contexte,
var et
obj sont le même objet (
var = (Var) obj ). La présence de la vérification
nulle implique que l'objet transmis peut être nul. Donc, appeler
égal (nul) se traduira par un NPE, au lieu du
faux attendu, juste à la première ligne. Oui, le chèque
est là, mais, malheureusement, il est au mauvais endroit.
Quelques autres cas similaires, où un objet est utilisé avant la vérification:
- V6060 La référence «valeur» a été utilisée avant d'être vérifiée par rapport à null. ParquetRecordReaderWrapper.java (168), ParquetRecordReaderWrapper.java (166)
- V6060 La référence 'defaultConstraintCols' a été utilisée avant d'être vérifiée par rapport à null. HiveMetaStore.java (2539), HiveMetaStore.java (2530)
- V6060 La référence 'projIndxLst' a été utilisée avant d'être vérifiée par rapport à null. RelOptHiveTable.java (683), RelOptHiveTable.java (682)
- V6060 La référence «oldp» a été utilisée avant d'être vérifiée par rapport à null. ObjectStore.java (4343), ObjectStore.java (4339)
- et ainsi de suite ...
Conclusion
Si vous vous êtes déjà intéressé au Big Data, ne serait-ce qu'un peu, alors vous pourriez à peine ignorer l'importance d'Apache Hive. Il s'agit d'un projet populaire et assez important, composé de plus de 6500 fichiers source (* .java). De nombreux développeurs l'ont écrit depuis de nombreuses années, ce qui signifie qu'il y a beaucoup de choses qu'un analyseur statique peut y trouver. Cela prouve une fois de plus que l'analyse statique est extrêmement importante et utile lors du développement de projets de moyenne et grande envergure!
Remarque Les vérifications ponctuelles comme celle que j'ai faite ici sont bonnes pour présenter les capacités de l'analyseur, mais sont un scénario totalement inapproprié de l'utiliser. Cette idée est développée
ici et
ici . L'analyse statique doit être utilisée régulièrement!
Cette vérification de Hive a révélé pas mal de défauts et de fragments suspects. Si les auteurs d'Apache Hive rencontrent cet article, nous serons heureux de vous aider dans la tâche difficile d'améliorer le projet.
Vous ne pouvez pas imaginer Apache Hive sans Apache Hadoop, donc la licorne de PVS-Studio pourrait bien rendre visite à celle-ci aussi. Mais c'est tout pour aujourd'hui. En attendant, je vous invite à
télécharger l'analyseur et à vérifier vos propres projets.