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

Closed
opened 3 years ago by JLuc · 16 comments
JLuc commented 3 years ago

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.

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.
Poster

ça expliquerait certaines explosions de cache

ça expliquerait certaines explosions de cache
b_b commented 3 years ago
Owner

Version cible 3.2, j'ai bon ?
Version cible mise à 3.2

Version cible 3.2, j'ai bon ? **Version cible mise à 3.2**
Owner

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

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
Poster

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.

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.
Poster

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

Prise de notes sur ce que font evaluer_fond, parametrer et recuperer_fond : https://contrib.spip.net/evaluer_fond-parametrer-et-sessions
Poster
There is no content yet.
Poster

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.

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.
Poster

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.

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.
Poster

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.

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.
Poster

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).

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).
Owner

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

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

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.

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
Poster

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]`.

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 <code class="php">[sessionnement_contaminant=>true]`.
Poster

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 :

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);

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);`
Poster

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)

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)
Owner

Appliqué par commit r24215.
Statut changé à Fermé

Appliqué par commit r24215. **Statut changé à Fermé**
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.