[BUG] FML : FMLSecurityManager

Mokona78

Et concrètement, elle sert à quoi cette fonction ?

J’ai regardé un peu, elle est appelée dans le cas qui pourrait lancer une exception lorsque l’on quitte le jeu.
Ca ne crash pas car à ce moment, la callstack est plus profonde que 6.

Mais…

1/ Pourquoi est-elle appelée ? / À quoi ça sert de vérifier dans quel contexte d’exécution on est ?
2/ Pourquoi est-ce qu’elle gêne dans les tests unitaires ?

SCAREX

Le FMLSecurityManager est là pour empêcher que les mods quittent le jeu ou bien changent le security manager.

Elle doit gêner pour les tests unitaires car ils doivent fermer l’application de force je pense

Mokona78

Mais qu’est-ce qui empêche les mods d’aller patcher le bytecode du Security Manager ?

RedRelay

1/ Elle permet de ne pas pouvoir changer le SecurityException de façon standard et de levé une exception dès qu’une classe autre que FML ou Minecraft appelle la fonction System.exit().
Bien que l’exception levé fasse crasher l’appli, elle permet de générer un rapport d’erreur. Ça éviter aux joueurs de signaler un bug sans crash log lorsqu’un moddeur cochon utilise System.exit() (et il y en a sur ce forum).

2/ Mon Test Runner JUnit custom, est lancé sur Eclipse via “org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main()”. C’est cette classe qui appel System.exit() à la fin de tout les tests.
Donc les test sont exécuté, mais j’ai un crash dans la console et j’aime pas, ça fait sale ^^

Rien n’empêche les mods d’aller patcher le bytecode du SecurityManager, c’est d’aller ce que j’ai fais ^^
(Mais c’est d’un bordel monstre avec les différents ClassLoader, ça fais 4 jours que je suis dessus, il y a plein de petite subtilité a savoir. C’est pas forcément compliqué, mais ce sont des choses qu’ils faut savoir ^^)

Je n’étais pas obliger de report le bug, mais tant qu’a faire ^^
Je doute que beaucoup de personnes ait l’occasion de dealer avec cette classe donc j’ai fais une pull request ^^
(Je vais passer contributeur MinecraftForge, la classe )

SCAREX

Normalement le Security manager de fml ne peut pas être patché puisqu’il est chargé avant les plugins fml

RedRelay

Je n’utilise pas de coremod pour patcher le SecurityManager.
En gros, dans le constructeur de mon TestRunner, voila ce que j’ai :


public MinecraftServerJUnitRunner(Class klass) throws InitializationError {
super(klass);
ASMTransformer.injectCode();
try {
Class.forName("GradleStartServer", true, Thread.currentThread().getContextClassLoader()).getDeclaredMethod("main", String[].class).invoke(null, new Object[] { params });
} catch (Throwable e) {
throw new InitializationError("Unable to launch Minecraft Server : "+e.getMessage());
}

System.out.println("Waiting for Minecraft Server start …");
/* ... */

Donc je fais les modifications avant de lancer le jeu.
Les classes sont bien modifié, mais je n’arrive pas a les lancer, à chaque fois je retombe sur les classes originale …
Donc pour l’instant je cherche… pourtant j’ai créer un nouveau ClassLoader pour avoir accès a la fonction defineClass(classname, byte[], offset, length)
Et je met ce classLoader dans Thread.currentThread().setContextClassLoader() …

Je modifie la class GradleStartCommon pour remplacer le Class.forName(String name) par Class.forName(String name, boolean init, ClassLoader cl)
Je modifie la class Launch pour que mettre mon CustomClassLoader en parent du nouveau ClassLoader et lui ajouter une classLoaderExcusion
Je modifie la class FMLSecurityManager en supprimant toutes les instructions dans checkPermission()

Ça a marché une fois il me semble, alors j’ai factorisé mon code pour que se soit plus propre et la ça ne fonctionne plus …
Je m’en tire les cheveux ><

Mokona78

@‘Blackout’:

Je n’étais pas obliger de report le bug, mais tant qu’a faire ^^
Je doute que beaucoup de personnes ait l’occasion de dealer avec cette classe donc j’ai fais une pull request ^^

C’est la bonne attitude !

Merci pour les précisions.

RedRelay

@‘Mokona78’:

@‘Blackout’:

Je n’étais pas obliger de report le bug, mais tant qu’a faire ^^
Je doute que beaucoup de personnes ait l’occasion de dealer avec cette classe donc j’ai fais une pull request ^^

C’est la bonne attitude !

Merci pour les précisions.

Parfois je me le demande, ma pull request a été refuser avec le message :
@‘cpw’:

[font=-apple-system, BlinkMacSystemFont,No error log? I’ve never seen this happen. Hypothetical fixes are not accepted.]

J’hallucine …

SCAREX

Ce que tu peux faire à la place c’est de re-créer le FMLSecurityManager et de charger le bytecode depuis un autre fichier

Mokona78

@‘Blackout’:

@‘Mokona78’:

@‘Blackout’:

Je n’étais pas obliger de report le bug, mais tant qu’a faire ^^
Je doute que beaucoup de personnes ait l’occasion de dealer avec cette classe donc j’ai fais une pull request ^^

C’est la bonne attitude !

Merci pour les précisions.

Parfois je me le demande, ma pull request a été refuser avec le message :
@‘cpw’:

[font=-apple-system, BlinkMacSystemFont,No error log? I’ve never seen this happen. Hypothetical fixes are not accepted.]

J’hallucine …

C’est celui qui a refusé qui n’a pas la bonne attitude…

Superloup10

cpw est quand même plus sympa avec les pull request que Lex, en revanche, les tests unitaires ne sont pas considérés comme une utilisation normale de FML et de Forge d’où le rejet.

Mokona78

Mais là, il y a une erreur non ? Elle n’est pas bien complexe à reproduire j’imagine. Il suffit d’appeler la fonction dans un contexte avec une pile moins profonde que 5 niveaux.

À moins que ce soit impossible ?

Superloup10

Si c’est une erreur très grave, il y a de forte chance que d’autres moddeurs aient eu le même souci, personnellement, j’ai beau avoir relue une bonne dizaine de fois le code qu’à donné Blackout, je ne vois toujours pas où est le problème.

Mokona78

Si getClassContext(); renvoie un tableau de 4 ou 5 éléments ?

Superloup10

Aucune idée, je ne sais pas ce que renvoie ce getter, un tableau de class, mais lesquelles ?

Mokona78

Ça renvoie la callstack. Ce sont les différentes appels qui ont amenés à l’appel courant.

Mais peu importe:

un tableau de taille 4 (>3) auquel on accède à l’élément d’indice 4, ça fait IndexOutOfBoundsException.
un tableau de taille 5 (>4) auquel on accède à l’élément d’indice 5, ça fait pas mieux, pas pire.

Le test n’est pas cohérent avec l’appel qui suit.

Après, peut-être qu’il y a une règle qui dit que si le tableau renvoyé est assuré d’avoir une taille 4 alors il a une taille 5. Et s’il a une taille 5 il est assuré d’avoir une taille 6… J’y crois assez peu.

robin4002

http://www.minecraftforge.net/forum/index.php/topic,40907.msg215989.html#msg215989
“So anyway, I have to completly bypass the security manager using ASM.”
Retires cette phrase avant que lex ne la voit, sinon tu vas encore plus te faire allumer x)
Lex n’aime pas du tout les “comme vous n’acceptez pas mon correctif / ma fonctionnalité je vais tout hacker de mon côté avec de l’ASM”.

Mokona78

C’est pour un test runner, pas pour distribuer dans un mod. Je ne vois pas pourquoi il ne pourrait pas le faire.

De plus, il ajoute le crash log demandé qui montre bien que le problème est réel et pas hypothétique.

Superloup10

cpw a fini par merge le pull request. (voir https://github.com/MinecraftForge/MinecraftForge/pull/3125#issuecomment-236043532 )

RedRelay

@‘robin4002’:

http://www.minecraftforge.net/forum/index.php/topic,40907.msg215989.html#msg215989
“So anyway, I have to completly bypass the security manager using ASM.”
Retires cette phrase avant que lex ne la voit, sinon tu vas encore plus te faire allumer x)
Lex n’aime pas du tout les “comme vous n’acceptez pas mon correctif / ma fonctionnalité je vais tout hacker de mon côté avec de l’ASM”.

Non non, ce n’est pas du tout ça dont il est question x)
Indépendamment de ma volonté, je fais un System.exit() et ce n’est pas autorisé par le SecurityManager.
Donc la, bug ou pas, je dois bypass ^^
Mais ce n’est pas pour un mod, donc aucun soucis incompatibilité ^^