Une optimisation Dovecot que le compilateur effaçait depuis le début

Une commande IMAP THREAD peut faire tomber un processus Dovecot avec un échec d'assertion, au fond du code de tableaux de libdovecot :

Panic: file array.c: line 10 (array_idx_modifiable_i):
  assertion failed: (idx < array->buffer->used / array->element_size)

Cette assertion, c'est la couche tableau qui attrape un accès hors limites : quelqu'un a demandé l'élément idx dans un tampon qui n'a pas idx + 1 éléments. Un contrôle de bornes. Il a sauté depuis un chemin de code qui, à première vue, venait juste d'allouer le tableau assez grand. Le plus intéressant, ce n'est pas le plantage. C'est le commentaire une ligne au-dessus, qui décrit une optimisation qui n'avait tourné dans aucun build optimisé depuis des années.

Ce que le code croyait faire#

La faute vit dans mail_thread_strmap_remap(), dans src/lib-storage/index/index-thread.c, la routine qui renumérote les nœuds de string-map que Dovecot utilise pour threader une boîte aux lettres. Elle construit un tableau de nœuds tout neuf et essaie d'être maligne :

i_array_init(&new_nodes, new_count + invalid_count + 32);
 
/* optimization: allocate all nodes initially */
(void)array_idx_modifiable(&new_nodes, new_count-1);
 
for (i = 0; i < old_count; i++) {
    if (idx_map[i] != 0) {
        node = array_idx_modifiable(&new_nodes, idx_map[i]);
        /* ... */
    }
}

L'intention se lit clairement. Initialiser le tableau, toucher le dernier indice une fois pour le forcer à sa pleine taille dès le départ, pour que les accès par itération dans la boucle n'aient jamais à réallouer. Une micro-optimisation raisonnable pour un chemin chaud.

Ça ne marche pas. Deux décomptes séparés, et ils se combinent en quelque chose de plus intéressant que chacun pris seul.

Décompte un : l'appel au tableau ne fait grandir rien du tout#

i_array_init() fixe la capacité du tableau mais laisse la longueur logique à zéro. Le tampon peut contenir new_count + invalid_count + 32 éléments, mais used == 0. Rien n'y est encore.

array_idx_modifiable() est le mauvais outil pour le faire grandir. Il retourne un pointeur en écriture vers un élément qui existe déjà et assure que l'indice est dans les bornes. Il n'étend pas used. Donc l'appeler avec un indice au-delà de la fin d'un tableau de longueur zéro, ce n'est pas une demande de croissance, c'est un accès hors limites, et l'assertion est là pour attraper exactement ça.

La boucle tourne contre un tableau encore logiquement vide. Dès que idx_map[i] != 0, l'accès à la ligne 200 demande un nœud qui n'est pas là, et le contrôle de bornes saute. Au moment du plantage, new_count valait 8 et used valait toujours 0.

#7  array_idx_modifiable_i (array=..., idx=<optimized out>) at array.c:10
#8  mail_thread_strmap_remap (idx_map=..., old_count=...,
                              new_count=8, ...) at index-thread.c:200
#9  mail_index_strmap_view_renumber (...) at mail-index-strmap.c:867
#10 mail_index_strmap_write (...) at mail-index-strmap.c:1192
#11 mail_index_strmap_view_sync_commit (...) at mail-index-strmap.c:1239
#12 mail_thread_index_map_build (...) at index-thread.c:359
#13 mail_thread_init (...) at index-thread.c:573
#14 imap_thread (...) at cmd-thread.c:90
#15 cmd_thread (...) at cmd-thread.c:282

Remarquez que la frame du plantage est index-thread.c:200, l'accès dans la boucle, et non :188, la ligne de l'« optimisation ». C'est le deuxième décompte, et c'est celui qui a pris une minute à voir.

Décompte deux : l'optimisation n'a jamais été là#

Si la ligne de pré-allocation faisait son travail, elle aurait fait grandir le tableau (ou planté elle-même) avant le début de la boucle. Elle n'a fait ni l'un ni l'autre. Dans un build optimisé, elle ne s'exécutait pas du tout.

array_idx_modifiable_i(), la fonction derrière la macro array_idx_modifiable(), est déclarée ATTR_PURE dans src/lib/array.h. ATTR_PURE est une promesse au compilateur : aucun effet de bord, le résultat ne dépend que de ses arguments. Cette licence permet au compilateur de supprimer un appel dont la valeur de retour est jetée, parce que par définition cet appel ne peut rien changer d'observable.

Et c'est exactement ce que le code original en faisait :

(void)array_idx_modifiable(&new_nodes, new_count-1);

Casté en void, jeté. Sous -O2, le compilateur lit la promesse ATTR_PURE, voit un résultat pur qui ne va nulle part, et retire la ligne. « optimization: allocate all nodes initially » a été du code mort dans chaque build optimisé jamais livré. Ni lent, ni planté nulle part. Juste pas là. Le commentaire décrit une intention que le binaire n'a jamais exécutée.

Le reproduire en six lignes#

Je voulais le prouver contre une libdovecot standard, rien qu'un mainteneur ne pourrait pas lancer lui-même. Tout le bug tient dans un main() :

#include "lib.h"
#include "array.h"
#include <stdio.h>
 
int main(void)
{
    lib_init();
 
    ARRAY(int) arr;
    i_array_init(&arr, 16);
 
    int *node = array_idx_modifiable(&arr, 1);
    printf("unreachable: node=%p\n", (void *)node);
    return 0;
}

Le détail clé : node = ... suivi du printf. Faire couler la valeur de retour à travers une vraie utilisation déjoue l'élision d'ATTR_PURE, donc l'appel survit même sous -O2. Ça reproduit la ligne 200, où la vraie panique saute, plutôt que la ligne 188, où elle se cachait. Lancez-le et vous obtenez la chaîne d'assertion exacte de la frame #7, à -O0 comme à -O2. Trois lignes de corps, tout le défaut.

Le même motif, i_array_init() suivi d'un array_idx_modifiable() jeté sur un indice cible, se trouve aussi dans mail_thread_root_thread_merge() dans index-thread-finish.c. Même bug latent, même optimisation invisible.

Le correctif était déjà dans le fichier#

Le bon appel est array_idx_get_space(). Pas d'ATTR_PURE, donc il ne se fait jamais élider, et il fait vraiment grandir le tampon via buffer_get_space_unsafe() au lieu d'assurer. Pour l'usage « donne-moi un pointeur vers l'élément à cet indice », c'est une généralisation stricte d'array_idx_modifiable() : sur un tampon déjà assez grand, les deux retournent le même pointeur, et sur un trop petit, array_idx_modifiable() assure tandis qu'array_idx_get_space() fait grandir. Échanger l'un pour l'autre ne change le contrat d'aucun appelant.

 	/* optimization: allocate all nodes initially */
-	(void)array_idx_modifiable(&new_nodes, new_count-1);
+	(void)array_idx_get_space(&new_nodes, new_count-1);

Je n'ai pas eu à inventer le correctif. Le bon idiome était déjà utilisé quelques centaines de lignes plus loin dans le même arbre de sources, dans index-thread-finish.c, sur le même genre de tableau, faisant le même travail que le commentaire cassé prétendait faire. Un site d'appel avait été écrit correctement et deux ne l'avaient pas été, et les deux mauvais l'étaient d'une façon que l'optimiseur recouvrait discrètement.

Ce que j'en retire#

ATTR_PURE n'est pas le méchant. Il est correct : array_idx_modifiable_i() est vraiment pure, jeter son résultat est vraiment inutile, et le compilateur a fait exactement la bonne chose en supprimant l'appel. Le problème, c'est qu'un commentaire n'est pas un test. « optimization: allocate all nodes initially » avait été faux pendant toute la vie du code, et rien n'avait échoué assez fort pour le dire, parce que l'accès hors limites latent ne mord que quand le tableau doit grandir au-delà de ce qu'une seule itération touche. Un appel jeté à une fonction pure est une non-opération que le compilateur a le droit d'effacer, et si ta seule preuve qu'un code tourne est qu'il est écrit, tu ne sais pas vraiment qu'il tourne.

Ça remonte en amont comme un correctif de pure correction de code : le reproducteur, la trace annotée, le patch de deux lignes. Chaque frame de cette trace est un symbole Dovecot, ce qui en fait un patch propre qu'un mainteneur peut vérifier en cinq minutes, au lieu d'un rapport vague que personne ne peut reproduire.