Project

General

Profile

Anomalie #4235

un cache sessionné contamine les caches non sessionnés suivant

Added by jluc - 11 months ago. Updated 9 months ago.

Status:
Fermé
Priority:
Normal
Assignee:
-
Category:
compilo
Target version:
Start date:
11/28/2018
Due date:
% Done:

100%

Resolution:
Navigateur:

Description

Dans une suite d'inclusion dynamique de squelettes, si l'un d'entre eux est sessionné, tous les squelettes suivants seront compilés comme sessionnés.

Jeu de test :

Squelette principal :

<INCLURE{fond=inclure/shouldbesafe}>
<INCLURE{fond=inclure/contaminant}>
<INCLURE{fond=inclure/shouldbesafetoo}>

noisette inclure/shouldbesafe :

HMMM ici ça devrait pas être sessionné !

noisette inclure/contaminant :

Vous êtes #SESSION{nom}

noisette inclure/shouldbesafetoo :

HMMMNNNNNN ici ça devrait pas être sessionné NON PLUS !

On constate que shouldbesafe est non sessionné et que contaminant est sessionné comme ils se doivent,
mais que shouldbesafetoo est sessionné alors qu'il devrait ne pas l'être.
Si on supprime l'inclusion de contaminant, shouldbesafetoo n'est pas sessionné.

Càd que la compilation de contaminant "contamine" le compilateur qui "contamine" shouldbesafetoo.

Dans le compilateur, c'est $GLOBALS['cache_utilise_session'] qui propage la nécessité de sessionner. Dans la fonction evaluer_fond, il est indiqué "il faut bien lever ce drapeau après avoir evalué le fond pour ne pas faire descendre le flag vers les inclusions appelees" : il semble que ça ne soit pas correctement fait dans tous les cas.

1. DEBUG_session_dynamique_avecfix.log View - trace d'une suite d'inclusion dynamique - avec le fix (8.29 KB) jluc -, 11/29/2018 11:28 AM

3. DEBUG_session_dynamique_sansfix.log View (8.35 KB) jluc -, 11/29/2018 11:28 AM

0. Sessionnage avec et sans fix1 et cible.ods - Sessionnements avec et sans fix1 et cible (16.5 KB) jluc -, 11/30/2018 09:09 AM

Associated revisions

Revision 24215 (diff)
Added by cerdic - 9 months ago

calcul des sessionnements : reinitialiser le flag entre 2 inclusions successives, il ne doit se propager qu'en remontant

Fix #4235 :
- les inclusions dynamiques sont toujours calculées à partir d'un contexte vierge de sessionnement
- les inclusions statiques sessionnées contamient leur contexte appelant

History

#1 Updated by jluc - 11 months ago

ça expliquerait certaines explosions de cache

#2 Updated by b b 11 months ago

  • Target version set to 3.2

Version cible 3.2, j'ai bon ?

#3 Updated by cedric - 11 months ago

Le principe c'est que contaminant lève le flag lors de son évaluation, pour que le squelette appelant sache à la fin de son évaluation qu'il contient un squelette sessionné, et qu'il est donc lui même potentiellement sessionné.
Le problème est que :
1/ si il est inclu avec #INCLURE le squelette appelant est de fait sessionné car contenant une copie de l'inclusion dans son cache.
2/ si il est inclu avec <INCLURE> le squelette appelant n'est pas sessionné, car son cache contiendra toujours la même chose (en très gros include(...)) donc ne nécessite pas de sessionnage

-> il faudrait donc que selon le type d'inclusion le drapeau soit levé ou non (un fix rapide peut être de ré-ecraser le flag avec sa valeur avant <INCLURE> après chaque <INCLURE>)

3/ quand on lève le drapeau ça renseigne bien le squelette appelant, mais potentiellement tous les squelettes qui sont évalués après levage du drapeau peuvent prendre le flag pour eux. Le problème vient du fait qu'on a pas de communication remontante.
-> une solution est qu'un squelette ne prenne en compte le flag que si il a été levé pendant sa propre compilation (en gros si le flag était déjà là à l'entrée on l'ignore à la sortie)

Voilà pour les idées de fix, après en pratique ça demande en effet pas mal de soin et de tests.

Et le mieux reste toujours et définitivement d'éviter les #SESSION et #AUTORISER qui sont fort utiles dans l'espace privé sans cache, mais se marient de toute façon très mal avec la notion de cache

#4 Updated by jluc - 11 months ago

A) Dans le bug de base tel qu'il est décrit, le pb c'est avec les INCLURE dynamiques (le point 2/ que tu évoques). Ces inclure dynamiques passent par recuperer_fond (et pas les inclusions statiques).

J'ai donc un fix dans recuperer_fond :

- sauvegarde l'état de la globale cache_utilise_session juste avant l'appel de evaluer_fond

        $sav_cache_utilise_session = (isset($GLOBALS['cache_utilise_session']) ? $GLOBALS['cache_utilise_session'] : null);
        unset($GLOBALS['cache_utilise_session']);
        $page = evaluer_fond($f, $contexte, $connect);


C'est simple et efficace : on arrête la contamination des squelettes inclus dynamiquement après levage du cache (évoquée dans ton 3/).

- puis restauration de létat de la globale cache_utilise_session à la fin de la séquence d'instructions (et aprés appel du pipeline recuperer_fond et de encoder_contexte_ajax - on sait jamais)

        $GLOBALS['cache_utilise_session'] = (isset($sav_cache_utilise_session) ? $sav_cache_utilise_session : null);
    } // fin du foreach ($fond)
    $GLOBALS['_INC_PUBLIC']--;

Est-on d'accord que les INCLURE DYNAMIQUES, et les recuperer_fond de manière générale, doivent toujours être faits de manière vierge de globale cache_utilise_session ?
Dans ce cas, ce fix peut à mon avis être commité.
Il règle le problème d'explosion du cache et donc de perte du cache.

Je le teste actuellement sans problème détecté et je constate avec XRay une énorme diminution des caches sessionnés !

Y aurait il des situations particulières à tester ?

B/ Je me demande s'il n'y a pas un autre problème (indépendant de ce fix) dans le cas d'une suite de 3 inclusions statiques, par exemple, dont la 2ème est sessionnée. À vérifier.

#5 Updated by jluc - 11 months ago

Prise de notes sur ce que font evaluer_fond, parametrer et recuperer_fond : https://contrib.spip.net/evaluer_fond-parametrer-et-sessions

#7 Updated by jluc - 11 months ago

Pour B/, c'est plus généralement le sujet du sessionnement d'une inclusion statique.

Une noisette elle-même non sessionnée lorsqu'elle est appelée directement (ou inclue dynamiquement) doit elle être sessionnée lorsqu'elle est inclue statiquement par un squelette sessionné ? Il me semble que non, ça n'a pas de sens.

Or, les inclusions statiques sont appelées avant que le squelette n'appelle sa fonction html_md5.

Du coup :

Avant le fix, les inclusions statiques de "contaminante" (2 eme inclusion de la page principale) étaient donc non sessionnées, alors que celles de l'inclusion dynamique suivante "shouldbesafetoo" étaient, elles, contaminées !! En effet, entre temps, "contaminante" avait appelé sa fonction html_md5. C'était visiblement absurde.

Avec le fix, les inclusions statiques en-elles-mêmes-non-sessionnées ne seront donc jamais sessionnées lorsque le squelette est appelé par un recuperer_fond (inclusion dynamique). Peut être même ne le seront elles jamais, ce qui est OK puisqu'elles sont en-elles-mêmes-non-sessionnées.

Dans les logs jolifiés uploadés ci dessus, les noisettes "contaminantes" et "shouldbesafetoo" (inclues dynamiquement à la suite) incluent chacunes 2 noisettes : l'une statiquement, l'autre dynamiquement. Un diff met en avant les différences de sessionnages.

#8 Updated by jluc - 11 months ago

Oups, la réponse est NON à la question plus haut de savoir si les recuperer_fond doivent toujours être faits de manière vierge de globale cache_utilise_session; puisque contrairement à ce que je croyais les inclures statiques passent AUSSI par recuperer_fond.

Mon patch va donc trop loin dans la suppression des caches sessionnés.

Concernant les inclusions statiques, j'ai l'impression que le comportement attendu dans une suite d'inclusions statiques si l'une d'elle seulement, au milieu, est sessionnée, c'est :
- Il est inutile de sessionner les autres inclusions statiques qui ne le sont pas en elle-même.
- Par contre il faut sessionner le squelette incluant.

Le patch assure le premier point mais pas le 2eme.
Pour ce 2eme point, il faudrait savoir si on est dans une inclusion statique (via une entrée de $options ?) et dans ce cas, re-lever le flag, avant de sortir de recuperer_fond, s'il a été levé par l'un des fonds de (array) fond.

#9 Updated by jluc - 11 months ago

Cedric, je ne sais pas pourquoi tu écris « Le problème vient du fait qu'on a pas de communication remontante ». Car on ce drapeau global assure bien une communication remontante et même dans tous les sens, ou du moins haut, bas et avant. Il n'y a qu'en arrière dans le temps qu'il ne leake pas, ce qui fait que les inclusions de même niveau aprés une inclusion sessionnée ne sont pas traitée de la mm manière que les inclusions avant.

#10 Updated by jluc - 11 months ago

Bilan d'étape

- comment se calcule le sessionnement ou non-sessionnement d'une noisette : https://contrib.spip.net/5065

- Le tableur ci-joint décrit 2 jeux de test pour tester les situations d'inclusions statiques et dynamiques
et présente dans chacun des cas la manière avec laquelle SPIP sessionne les noisettes,
ce qu'obtient le fix1 proposé précédemment,
et la manière cible selon moi (= quand il n'y a pas de bug)
- avec le core (3 + 3 erreurs)
- avec le fix1 (1 erreur : cas d'une inclusion statique contaminante)
- la cible dans l'idéal (à mon avis).

#11 Updated by marcimat 🌈 11 months ago

Je pense que ta colonne cible est correcte, d’après tes tableaux.

#12 Updated by jluc - 11 months ago

Voilà j'ai un fix2 pour a priori toutes les combinaisons d'inclusions statiques et dynamiques, sessionnées ou non, telles que testées par le jeu de squelette de test (décrit dans le tableur).
Pour cela, 2 fonctions sont ajustées :
  • balise_INCLURE_dist passe une nouvelle option 'session_contaminante' à recuperer_fond
  • recuperer_fond protège le contexte du cache appelant sauf dans le cas où le cache courant est sessionné et qu'il y a l'option session_contaminante (présente pour une inclusion statique)
Ainsi :
  • les inclusions dynamiques sont toujours calculées à partir d'un contexte vierge de sessionnement
  • leur état sessionné ou non n'a pas de conséquence sur le contexte appelant ou sur leurs voisines
  • les inclusions statiques sessionnées contaminent leur contexte appelant mais pas leurs soeurs d'inclusion

Le calcul de sessionnement des différentes noisettes se fait bien tel que énoncé dans la colonne "cible" du tableur.

- .diff : https://patch-diff.githubusercontent.com/raw/spip/SPIP/pull/32.diff
- jolie PR : https://github.com/spip/SPIP/pull/32

#13 Updated by jluc - 11 months ago

Il faut encore tester comment est gérée l'inclusion des modèles par appel dans un contenu éditorial et par #MODELE, et peut être patcher #MODELE de même que l'a été #INCLURE avec l'ajout de [sessionnement_contaminant=>true].

#14 Updated by jluc - 11 months ago

Le cas des #MODELES et des <modeles> a été géré de la même manière. C'est Fix3 qui est fusionné aux fix1 et fix2 dans la PR sur github :
- le .diff : https://patch-diff.githubusercontent.com/raw/spip/SPIP/pull/32.diff
- la PR : https://github.com/spip/SPIP/pull/32

C'est plein de petits commits car j'édite fichier après fichier sur github ! Mais au final c'est très peu de lignes changées.

Attention : il y a aussi un fix dans textwheel que je ne peux pas inclure dans ce PR : dans le code de traiter_modele,php dans textwheel/inc/lien.php ( https://zone.spip.net/trac/spip-zone/browser/spip-zone/_core_/branches/spip-3.2/plugins/textwheel/inc/lien.php#L861 ), il faut remplacer
$modele = recuperer_fond("modeles/dist", $contexte, array(), $connect);
par
$modele = recuperer_fond("modeles/dist", $contexte, array('sessionnement_contaminant'=>true), $connect);

#15 Updated by jluc - 9 months ago

Un plugin permet de tester le fix : https://plugins.spip.net/cachefix.html

Il surcharge 2 fichiers du dossier public/ avec leur version patchée selon la branche de SPIP, mais il faut aussi surcharger inc/utils.php "à la main" avec la version fournie (instructions dans le readme du dossier inc/ du plugin)

#16 Updated by cerdic - 9 months ago

  • Status changed from Nouveau to Fermé
  • % Done changed from 0 to 100

Appliqué par commit r24215.

Also available in: Atom PDF