Raydium 3D Game Engine

Official forum for everything about Raydium, ManiaDrive, MeMak, ...
It is currently Sun Apr 28, 2024 9:27 am

All times are UTC




Post new topic Reply to topic  [ 15 posts ] 
Author Message
PostPosted: Sun May 21, 2006 7:30 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
Pour des problèmes de sécurité et de souplesse, une discussion a été lancée par guitou par messages privés. Je vais tenter de la retranscrire ici.


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 7:31 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
Message de guitou :
Salut,

Est ce qu'il existe une spéc ou une doc plus précise que l'apireference pour ce qui concerne le fonctionnement du moteur réseau (notamment pour toutes les fonctions taguées "internal use") ?
Je pense qu'il y a un certain nombre de choses à revoir au niveau sécu mais je préfèrerais casser le moins de choses possibles (vis à vis du comportement actuel des fonctions).

Il serait surement intéressant de se fixer un format pour les paquets (ce qui est déjà plus ou moins le cas), une struct permettrait de gagner en lisibilité.

Est il vraiment nécessaire d'envoyer des paquets de taille fixe en udp ?
Je ne vois pas particulièrement de problème à envoyer des paquets de taille variable avec une vérification de cohérence dans la fonction raydium_network_read, histoire de se prémunir d'éventuels paquets forgés.

Y a t'il des contraintes au niveau ode concernant les données réseau échangées ?


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 7:32 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
- L'api ref est la seule doc ... Il faudrait effectivement faire quelque chose de plus précis, le wiki étant surement le meilleur endroit pour ça. Et tu as trèèèès probablement raison quand aux problèmes de sécu que l'on risque de croiser par là bas.

- Pourquoi pas une structure, effectivement. Sur le même thème, je me suis souvent dit qu'il fallait fournir à l'utilisateur de Raydium des fonctions (toutes simples) pour "pusher" ses données dans un paquet et ainsi éviter les inesthétiques memcpy et décallages d'offset demandés à l'heure actuelle.

- La taille fixe des paquets facilite (beaucoup) l'écriture de la couche réseau mais n'est pas foncièrement indispensable. Par contre le gain de perf avec des tailles variables risque d'être assez faible à mon avis (puisque on croise une majorité de paquets de données ODE sur le réseau en fait).


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 7:33 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
Message de guitou :
Xfennec wrote:
- Pourquoi pas une structure, effectivement. Sur le même thème, je me suis souvent dit qu'il fallait fournir à l'utilisateur de Raydium des fonctions (toutes simples) pour "pusher" ses données dans un paquet et ainsi éviter les inesthétiques memcpy et décallages d'offset demandés à l'heure actuelle.


Bon avant tout, je vais proposer un truc mais ne crie pas si je dis des conneries ou si ça implique trop de changements autre part :)
En fait, j'envisageais une structure raydium_network_packet de ce genre là:

Code:
struct raydium_network_packet
{
  unsigned int cur_read;
  unsigned int tot_size;
  /* Le paquet lui même */
  unsigned char type;
  unsigned char userid;
  unsigned short tcpid;
  char[RAYDIUM_NETWORK_DATA_SIZE] data;
}


Cette structure pourrait être allouée et remplie initialement avec une fonction raydium_network_new_pkt(unsigned char type, unsigned char userid, unsigned short tcpid). La taille du champ data est en fait RAYDIUM_NETWORK_PACKET_SIZE - la taille des champs type, userid et tcpid.
La structure serait ensuite manipulée avec raydium_network_write_data(struct raydium_network_packet * pkt, void * data, size_t size). Cette fonction irait écrire dans la partie data:
- en pkt->data+pkt->cur_read, on écrit la taille de la donnée qui va suivre
- en pkt->data+pkt->cur_read+sizeof(size_t), on écrit la donnée elle même
Enfin, on augmente l'index pkt->cur_read de la longueur écrite.

Il suffit ensuite de passer &pkt->type avec une longueur RAYDIUM_NETWORK_PACKET_SIZE à la fonction d'envoi dans la socket.

De l'autre coté, le packet est reçu, on initialise un paquet avec les données reçues. Puis raydium_network_read_data(struct raydium_network_packet * pkt, void * ptr, size_t * size) entre en jeu. Cette fonction va lire:
- en pkt->data+pkt->cur_read, la taille de la donnée qui doit être lue. Si cette taille est cohérente avec la taille des données encore à lire en se servant de pkt->cur_read et pkt->tot_size (i.e. pas de débordement), on va la mettre dans size (argument passé par référence)
- en pkt->data+pkt->cur_read+sizeof(size_t), la donnée elle même pour la mettre dans ptr (passé en argument).
Si tout s'est bien passé, l'index pkt->cur_read est augmenté de la taille lue.

Cette méthode est un peu lourde, on perd en charge utile également dans les paquets qu'on échange, on risque également de perdre en flexibilité de l'api (et j'entends presque des collègues hurler "asn1 !!" :)).
Cependant, elle me semble la plus simple pour éviter des "petites horreurs" que j'ai vues (strcpy et strlen sur des données venant d'un paquet réseau, c'est maaal :)).

Xfennec wrote:
- La taille fixe des paquets facilite (beaucoup) l'écriture de la couche réseau mais n'est pas foncièrement indispensable. Par contre le gain de perf avec des tailles variables risque d'être assez faible à mon avis (puisque on croise une majorité de paquets de données ODE sur le réseau en fait).


Je ne vois pas trop ce qui est facilité, à part le calcul de taille des données échangées. Avec la structure précédente, il suffirait de manipuler ->tot_size.


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 7:35 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
L'idée me va tout à fait. Je ne sais pas si c'est à toi que j'en parlais, mais j'ai déjà dit que la couche réseau de Raydium était très naïve, de par son histoire. Ce que je voulais dire à propos des tailles fixes de paquets, c'est que ca facilite (à mon avis) les opérations de stockage et de manipulation de l'info. Mais encore une fois, tant qu'a changer la couche à ce point, autant changer ce fait :)

En revanche, l'idée d'avoir la "taille de l'info à venir" juste avant l'information elle même ne me semble pas apporter grand chose (sans compter effectivement la perte d'espace). Si on recoit un paquet d'un certain type, on doit être capable de le décoder. Et cela ne me semble pas être plus "sécurisant", puisqu'il est facile de mentir sur cette taille dans un paquet forgé.
Pour préciser l'idée a laquelle je pensais pour offrir des "helpers" à l'utilisateur de l'API, il serait intéressant de les typer (raydium_network_write_int, char, string, ...) ce qui permet au passage de s'occuper de l'endiannes si nécessaire et de débarasser l'utilisateur du problème de taille de son info. Je ne sais pas si c'est clair :)


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 7:36 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
Message de guitou :
Xfennec wrote:
En revanche, l'idée d'avoir la "taille de l'info à venir" juste avant l'information elle même ne me semble pas apporter grand chose (sans compter effectivement la perte d'espace). Si on recoit un paquet d'un certain type, on doit être capable de le décoder. Et cela ne me semble pas être plus "sécurisant", puisqu'il est facile de mentir sur cette taille dans un paquet forgé.


Le fait de préciser la taille de la donnée dans le paquet permet lors d'un "read data" de savoir exactement combien de données on va copier. Si le paquet est forgé mais la taille de la donnée rentre dans la taille du paquet (autrement dit, la taille est correcte), on n'aura pas de débordement. Dans ce cas là, on écrit peut être des données incorrectes mais la faille que le forgeur tente d'exploiter ne sera plus au niveau raydium mais au niveau de l'application exploitant la donnée (j'espère que tu me suis :)).
C'est vraiment les débordements dans la couche raydium que je vise à éviter avec cette méthode.

Xfennec wrote:
Pour préciser l'idée a laquelle je pensais pour offrir des "helpers" à l'utilisateur de l'API, il serait intéressant de les typer (raydium_network_write_int, char, string, ...) ce qui permet au passage de s'occuper de l'endiannes si nécessaire et de débarasser l'utilisateur du problème de taille de son info. Je ne sais pas si c'est clair :)


Si si, c'est très clair, mais je pense qu'on réinvente asn1 à ce moment là.


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 7:51 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Bon j'ai dit une connerie, c'est surement pas de l'asn1 :) mais ça se résume à du tlv (type, longueur, valeur). On perd en place, certes, mais on connait la taille des données échangées et on peut donc s'affranchir des débordements.

On peut tout de même laisser l'accès au contenu du paquet pour les codeurs n'aimant pas ce genre d'enrobage :).

Voilà, les commentaires sont les bienvenus.

--
guitou


Top
 Profile  
 
 Post subject:
PostPosted: Sun Jun 04, 2006 12:45 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Bon, j'ai commencé à coder un petit truc pour réfléchir (oui désolé, je réfléchis mieux en codant :)), c'est pas encore stabilisé et c'est surement encore buggué mais ça devrait donner une idée de ce que je compte proposer.
Pour une vue "user", le plus simple est de regarder test_send.c et test_recv.c.

http://grrr.releeshan.net/websvn/listing.php?repname=tlv&path=%2F&sc=0
http://grrr.releeshan.net/websvn/filedetails.php?repname=tlv&path=%2Ftrunk%2Ftest%2Ftest_recv.c
http://grrr.releeshan.net/websvn/filedetails.php?repname=tlv&path=%2Ftrunk%2Ftest%2Ftest_send.c
--
guitou


Last edited by guitou on Wed Jun 07, 2006 6:15 am, edited 1 time in total.

Top
 Profile  
 
 Post subject:
PostPosted: Sun Jun 04, 2006 2:45 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
L'idée générale me semble tout à fait OK (et bravo pour ce code), mais mazette, quelle complexité !
Le fichier tlv.c illustre assez bien cet aspect, je trouve.
Je persiste à ne pas comprendre ou est l'intérêt d'intégrer la structure du paquet dans le paquet lui même (ce qui est AMHA la source de la "lourdeur" relative du code présenté). Je m'explique :
Quote:
Le fait de préciser la taille de la donnée dans le paquet permet lors d'un "read data" de savoir exactement combien de données on va copier. Si le paquet est forgé mais la taille de la donnée rentre dans la taille du paquet (autrement dit, la taille est correcte), on n'aura pas de débordement.


C'est aussi le cas pour une lecture du paquet sans ces infos "embarquées". Je sais que ce paquet est de type X donc je sais à l'avance quelle va être la structure de l'info à l'intérieur. Si je dois lire un entier, une chaine ou n'importe quoi d'autre, il est tout à fait possible de vérifier avant que cette lecture ne va pas déborder de la taille du paquet.

Je n'ai pas bossé autant que toi sur ce sujet (et de loin) mais je vois plutôt l'API sous cette forme (les noms des fonctions et structures sont à repenser) :

Code:
// structures utilisées en interne par Raydium, cf commentaires fonctions
typedef struct packet {
   signed char state;
   char *buff;
   int offset;
   int size; // info qui arrive de recvfrom
} packet;
packet packets[...];

// partie "écriture"
int ...new_packet(struct sockaddr *to, int from, signed char type, char *buff); // on respecte la forme originale de l'API. buff est un espace mémoire (qui appartient à l'application) qui va être "peuplé" par cette fonction et les suivantes. On retourne un handle qui est en fait un identifiant d'une structure (qu'on passe en state=1)
signed char ...push_int(int handle, int value); // on gère l'endianness et on incrémente l'offset
signed char ...push_char(int handle, char value); // ...
signed char ...push_float(int handle, float value); // ...
signed char ...push_str(int handle, char *value); // ...
signed char ...send(int handle); // on libère la structure "packet" (state=0)

// partie "lecture"
int ...decode_packet(char *buff); // on donne à cette fonction le paquet que l'on vient de recevoir, elle retourne un handle sur packets ... [NB: ça me plait pas en l'état]
signed char ...pop_int(int handle, int *value); // on gère l'endianness et on incrémente l'offset
signed char ...pop_char(int handle, char *value); // ...
signed char ...pop_float(int handle, float *float); // ...
signed char ...pop_str(int handle, char *value); // ...
signed char ...decode_finish(int handle); // on libère la structure [même remarque que decode_packet()]


Ce genre d'API à l'avantage de rester dans la logique Raydium, de continuer à responsabiliser l'application en ce qui concerne la gestion mémoire des "buff" (comme c'est le cas actuellement) et au passage de fortement faciliter la migration du code actuel. La gestion des débordement est à mon sens aussi correcte qu'avec un code qui embarque les infos sur la structure du paquet. Et le tout doit être plié en moins de 200 lignes de code.

Le seul problème étant ici d'arriver à faire passer l'info de taille de paquet de raydium_network_read() (qui l'obtient par recvfrom()) à ...decode_packet().

Je ne sais pas si ce message est clair ou non, et je tiens à répèter que je ne cherche qu'a faire avancer le débat sur ce sujet, et non à imposer mes vues :)


Top
 Profile  
 
 Post subject:
PostPosted: Sun Jun 04, 2006 3:27 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Désolé, j'ai toujours été compliqué dans ma tête. Je pense d'ailleurs que la complexité réside surtout dans le cache que j'ai voulu tester :). Je n'aurais pas dû me lancer dans ça ...

Bon, je vais tenter d'être plus clair concernant cette histoire de longueur.

Si on n'échange qu'une seule donnée dans un paquet, je comprends tout à fait que la longueur soit inutile. La valeur rendue par recvfrom suffit amplement à calculer la taille de notre donnée. Je ne peux qu'aller dans ton sens dans ce cas.
De même, si on échange plusieurs données chacune de taille fixe dans un paquet, on peut se débrouiller pour retomber sur ces pieds. On perd un peu en souplesse à mon avis: chaque interlocuteur doit connaître explicitement le format exact du message; si le format change, les différentes versions deviennent incompatibles. Mais, dans ce cas aussi, la longueur est inutile.

Cependant, lorsqu'on regarde des portions de code comme notamment dans raydium_network_read (raydium/network.c):

Code:
   memcpy(&version,buff+dec,sizeof(version));
   dec+=sizeof(version);

   app_or_mod=buff+dec;
   dec+=(strlen(app_or_mod)+1);
   
   // else -> id not found -> test game+version
   if(version != raydium_network_beacon_search.version ||
      strcmp(app_or_mod,raydium_network_beacon_search.app_or_mod))
      return(RAYDIUM_NETWORK_DATA_NONE); // not for us ...

   name=buff+dec;   
   dec+=(strlen(name)+1);


On peut voir ici que le paquet récupéré est censé embarquer deux données à taille variable. Si on ne passe pas la taille dans le paquet, je ne vois pas comment on peut deviner la taille exacte. Le strlen() n'est pas une bonne solution pour moi, il est facile de forger un paquet sans \0. Je n'arrive pas à voir de solution pour deviner la taille d'une donnée à partir de la donnée elle même. As tu une idée pour gérer ce problème ?

--
guitou


Top
 Profile  
 
 Post subject:
PostPosted: Sun Jun 04, 2006 3:46 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
Tout n'est qu'une question de confiance accodée ou non. A partir du moment ou l'on décide de ne plus faire confiance aux données du paquets (ce qui n'est pas le cas dans le code actuel, je pense que nous sommes d'accord), que l'information de taille d'une information dans un paquet soit donnée selon ta méthode (par un entier préfixe) ou par un 0 terminal, le problème est le même : l'info n'est pas fiable et a peut être été forgée.

Pour l'exemple que tu donnes, au lieu de faire un strcpy/strlen/str* on passera par "pop_str()" dont le métier est bien de nous récupérer une (et une seule) chaine en cherchant le \0 ... ou la fin du paquet si quelqu'un tente de forger un paquet "sans fin". Dans l'extrait que tu donnes, on s'en tire avec 3 pop (un entier et 2 chaines) sans se poser plus de questions. S'il manque le 0 terminal à la première chaine, eh bien cette chaine sera "coupée" juste à la fin du paquet et le 2éme appel va donner une chaine vide.

Quote:
On perd un peu en souplesse à mon avis: chaque interlocuteur doit connaître explicitement le format exact du message; si le format change, les différentes versions deviennent incompatibles.

Oui. Les jeux utilisent tous ce genre de systèmes (d'ou les versions de protocoles et la nécessité systématique de patcher son jeu pour continuer à jouer en réseau). Si on souhaite de la souplesse à ce niveau, on passe tout par SOAP et WSDL ;)


Top
 Profile  
 
 Post subject:
PostPosted: Sun Jun 04, 2006 4:10 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Mmm, ok je vais implémenter ça, c'est vrai que ça reste bien plus simple à intégrer avec le code actuel. Je garde ma structure tlv sous le coude, on sait jamais, elle peut servir à autre chose (ça aura occupé mon week end :)).


--
guitou


Top
 Profile  
 
 Post subject:
PostPosted: Sat Jun 10, 2006 2:13 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Plop,

Xfennec wrote:
Code:
int ...new_packet(struct sockaddr *to, int from, signed char type, char *buff); // on respecte la forme originale de l'API. buff est un espace mémoire (qui appartient à l'application) qui va être "peuplé" par cette fonction et les suivantes. On retourne un handle qui est en fait un identifiant d'une structure (qu'on passe en state=1)
signed char ...send(int handle); // on libère la structure "packet" (state=0)

Je ne pense pas que la struct sockaddr *to est vraiment sa place dans new_packet. Je vois plutot la chose genre on initialise un packet avec new_packet(...) qui nous retourne un handle, on manipule le paquet puis on décide de l'envoyer avec send_packet(int handle, struct sockaddr *to).

Xfennec wrote:
Code:
int ...decode_packet(char *buff); // on donne à cette fonction le paquet que l'on vient de recevoir, elle retourne un handle sur packets ... [NB: ça me plait pas en l'état]

Là, je verrais bien la chose en deux étapes. On fait d'abord un new_packet(0, 0, buff); qui nous retourne un handle de paquet puis on appelle recv_packet(int handle); qui remplit la partie data du paquet et prépare le décodage.

Xfennec wrote:
Code:
signed char ...decode_finish(int handle); // on libère la structure [même remarque que decode_packet()]

Ici, je verrai une fonction free_packet(int handle) qui ne se charge que de faire passer l'état du paquet à "libre". Du coup, la fonction send_packet() ne devrait se charger que de l'envoi, la libération du paquet devrait être explicite avec un free_packet().

Pour résumer:
Code:
int new_packet(signed char type, signed char uid, void * buffer, size_t bufsize);
int send_packet(int handle, struct sockaddr * sock_to);
int recv_packet(int handle, struct sockaddr * sock_from);
int free_packet(int handle);

Les appels à send_packet et recv_packet doivent ainsi être encadrés par des new_packet() et free_packet().

Pour ce qui est des autres fonctions push_* et pop_*, aucune remarque particulière.
Pour la partie "header" du paquet (i.e. type, userid, tcpid), une structure serait pê la bienvenue tout en restant invisible à l'utilisateur. Il suffirait de rajouter des accesseurs si le besoin apparaissait.

Qu'en penses tu ?


--
guitou


Top
 Profile  
 
 Post subject:
PostPosted: Sat Jun 10, 2006 7:15 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Bon ça donne un truc de ce genre là :)

http://grrr.releeshan.net/websvn/filedetails.php?repname=rezal&path=%2Ftrunk%2Fsrc%2Fpacket.c&sc=1
http://grrr.releeshan.net/websvn/filedetails.php?repname=rezal&path=%2Ftrunk%2Fsrc%2Fpacket.h&sc=1
http://grrr.releeshan.net/websvn/filedetails.php?repname=rezal&path=%2Ftrunk%2Ftest%2Ftest_recv.c&sc=1
http://grrr.releeshan.net/websvn/filedetails.php?repname=rezal&path=%2Ftrunk%2Ftest%2Ftest_send.c&sc=1


--
guitou


Top
 Profile  
 
 Post subject:
PostPosted: Sun Jun 11, 2006 3:18 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
Ta vision des choses semble particulièrement "objet", avec les étrangetés inhérentes à cette orientation pour un "utilsateur type Raydium" (qui cherche avant tout une API simple, par envie ou nécessité) qui risque de se poser ce type de questions :
"Pourquoi dois-je créer un nouveau paquet alors qu'on est en train d'en recevoir un ?", par exemple (je fais référence au new_packet() avant le recv_packet() ici). C'est "informatiquement logique", mais pas très intuitif, si ? :) Idem pour le nom "recv_packet" (le paquet est déjà reçu).
En fait, pour faire court, je ne comprend pas le besoin de passer par deux fonctions pour la partie réception.

Pour la partie émission, la présence du sockaddr à la création du paquet ou lors de l'envoi en lui même ne me semble pas changer grand chose, mon idée était plutot de se débarrasser au plus tôt de tout ce qui ne concernait pas le corps du paquet :) (la partie "pas drôle" de l'envoi, toujours du point de vue utilisateur). Les deux solutions sont OK pour moi, en clair.

Enfin, je devais pas avoir les yeux en face des trous lors de l'écriture de mes remarques "ça me plait pas en l'état", puisqu'il est tout à fait possible de pousser les données un octet plus (on incrémente RAYDIUM_NETWORK_PACKET_OFFSET) et d'y glisser la taille donnée par recvfrom() ce qui régle complétement le problème. En plus, il est possible d'évacuer directement les paquets reçus dont ce champs ne correspond pas à la valeur donnée par recvfrom(), signe d'un paquet forgé (un peu vite).

J'avoue avoir du mal à exprimer tout ça sans écrire de code, je comprendrais que ce post ne soit pas clair du tout :)


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 15 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 186 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group