question sur un client c smtp

Le 01/05/2012 02:27, fakessh a écrit :

je suis débutant et j aimerais recevoir le maximum de conseils sur mon
ecriture

En vrac, quelques idées (attention, certaines sont peut être plus ou
moins importantes, mais amha pertinents tout de même):

- pour con628() je ne me serai pas embêté:

char base64char(char c)
{
static const char *const base64chs "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
assert(c >= 0 && c < 64);
return s[c];
}

- j'aurais accessoirement changé tous les "char" par des "unsigned
char", cela évite les casts pour l'arithmétique (oui, il faut du coup
des casts pour les "", mais c'est moins chiant que les erreurs
indétectables issues de char négatifs!)

- utiliser une structure intermédiaire et des tampons n'a à priori pas
un intérêt formidable pour l'encodage (ce n'est guère plus clair), moi
j'aurais écrit (non compilé, coquilles à prévoir)

...
int dst;
const unsigned char* src;

/* Loop through buffer, 3 bytes at once */
for(src = buf128, dst = 0 ; len > 2 ; len -= 3, src += 3) {
const unsigned int val (unsigned int) src[0] << 16)
| ((unsigned int) src[1] << 8)
| (unsigned int) src[2];

/* Convert into 4 base64 bytes */
dbuf[dst++] = base64char[(val >> 18) & 0x3f);
dbuf[dst++] = base64char[(val >> 12) & 0x3f);
dbuf[dst++] = base64char[(val >> 6) & 0x3f);
dbuf[dst++] = base64char[(val >> 0) & 0x3f);
}

/* Last bytes if any */
switch(len % 3) {
case 1:
{
const unsigned int val ((unsigned int) src[0] << 16);

dbuf[dst++] = base64char[(val >> 18) & 0x3f);
dbuf[dst++] = base64char[(val >> 12) & 0x3f);
dbuf[dst++] = '=';
dbuf[dst++] = '=';
}
break;
case 2:
{
const unsigned int val ((unsigned int) src[0] << 16)
| ((unsigned int) src[1] << 8);

dbuf[dst++] = base64char[(val >> 18) & 0x3f);
dbuf[dst++] = base64char[(val >> 12) & 0x3f);
dbuf[dst++] = base64char[(val >> 6) & 0x3f);
dbuf[dst++] = '=';
}
break;
default:
break;
}
...

- accessoirement il manque des tests pour vérifier que la destination ne
déborde pas au besoin (mais c'est inutile, si le buffer est alloué
avant, vu que l'on connait la taille finale)

- le dialogue avec le serveur est un peu brut de fonderie ; les memset
sont inutiles (recv_line devrait tester le nombre d'octets renvoyés par
SSL_read() et coller un si le nombre est positif (pas une erreur)) ;
aucun tests d'erreur n'est fait ; et aucun test du code côté serveur
n'est fait (le serveur renvoi un code qu'il est bon de tester pour
chaque commande ; voir (1) et (2) ; attention, ignorer les lignes côté
serveur qui sont sous forme "NNN-" et ne prendre que la dernière ligne
"NNN ") ; et enfin du debug serait probablement utile pour les tests
(envoyer sur stderr ce qui est reçu/envoyé avec des >>> et des <<< en
préfix par exemple) ; en clair avoir quelque chose au final comme:

if (send_clear(sockfd, "EHLO localhost") == 250
/* STARTTLS */
&& send_clear(sockfd, "STARTTLS") == 220
/* Re-issue EHLO */
&& send_ssl(ssl, "EHLO localhost") == 250
/* Command ok ; request username */
&& send_ssl(ssl, "AUTH LOGIN") == 334
/* User ; go ahead */
&& send_ssl(ssl, login_base64) == 334
/* Pass ; successful */
&& send_ssl(ssl, password_base64) == 335
/* Mail from */
&& ...
...
) {
..
}

(1) http://tools.ietf.org/html/rfc2822
(2) http://tools.ietf.org/html/rfc2554
.. et tester a la main aussi (telnet), ça permet souvent de comprendre
beaucoup plus vite qu'une RFC [digression: c'est la beauté des
protocoles RFC du reste: pas besoin de lire la spec au début, il est
possible de bricoler en jouant en direct avec le serveur -- essayez-donc
de faire ça avec une spec ISO, et vous allez vous tirer une balle dans
la tête assez vite]

- isoler les parties résolution DNS dans des fonctions, idem pour la
partie, SSL

- plein d'autres trucs sans doute que j'ai laissé passer, mais il y a
déjà des choses à piocher

(Coquilles et erreurs fortement possibles dans le code écrit un peu à la
va-vite)

In article <jnnakn$k66$1@speranza.aioe.org>,
fakessh <serge@smtp.fakessh.eu> wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

bonjour les gens du C Fu en france

je suis debutant en c
et je viens de commencer à ecrire un petit client smtp en c

https://github.com/fakessh/openprojectssl/blob/master/smtp.c
https://github.com/fakessh/openprojectssl/blob/master/smtp.h

je suis débutant et j aimerais recevoir le maximum de conseils sur mon
ecriture

Si tu es debutant, change de projet, il y a des tonnes de choses
interessantes a faire qui seront plus simples.

A moins d'avoir des tendances masochistes poussees, demarrer du C avec
un projet qui implique du reseau (et donc tout ce qui peut foirer) et
des chaines de caracteres, sans compter ssl, ca me parait une mauvaise
idee...

fakessh écrivit :

bonjour les gens du C Fu en france

Tiens, je suis exclus une fois...

je suis debutant en c
et je viens de commencer à ecrire un petit client smtp en c

https://github.com/fakessh/openprojectssl/blob/master/smtp.c
https://github.com/fakessh/openprojectssl/blob/master/smtp.h

Tiens, du réseau et du SSL dans un groupe C, est-ce vraiment en charte ?

je suis débutant et j aimerais recevoir le maximum de conseils sur mon
ecriture

J'ai regardé que le .h avant le .c ;

typedef short u_int16_t;
Type aujourd'hui obsolète, utilise plutôt uint16_t à la place ;
l'avantage, c'est que tu auras la définition correcte gratuitement !

#define OS_SUNOS5
//...
#elif defined(__GNUC__) && (defined(__APPLE_CPP__) ||
defined(__APPLE_CC__) || defined(__MACOS_CLASSIC__))
//...
#elif defined(__GUNC__) && defined(__linux__)
//...
#else
#error "Unsupported operating system"
D'abord, c'est une audience francophone (voire même française), donc il
n'y a pas de raisons d'utiliser de l'anglais, surtout dans un message
qui n'est pas sensé arrivé.
Ensuite, tu peux décaler (les franglophones disent «indenter») les
instructions #error ou #define pour faire ressortir la structure logique
des #if/#else/#endif, cela fait plus de 20 ans que les préprocesseurs C
regardent plus loin que la première colonne.
En tant qu'utilisateur d'autres compilateurs que GCC ou compatibles, je
me sens exclus, une fois de plus.
Enfin, "fakessh" propose un projet OpenSSL pas supporté sur OpenBSD, et
tu demandes de l'aide sur un groupe que lit un des contributeurs
français majeurs de OpenBSD, je crois que tu cherches les ennuis ;-)


#define OS_LINUX && OS_UBUNTU

Tu définis la macro «OS_LINUX» comme devant être remplacée à chaque
occurrence par les deux lexèmes «&&» suivi de «OS_UBUNTU».
ÀMHA tu voudrais plutôt
#define OS_LINUX oui
#define OS_UBUNTU oui


Ensuite j'ai regardé le .c ; et première surprise, tout ce qui écrit
ci-dessus est complètement inutile : le .h n'est pas #inclus par le .c
On trouve même
#ifdef WIN32
qui est un indice de non-concordance absolue...

#define CHK_NULL(x) if ((x)==NULL) exit (1)
Prend l'habitude d'écrire les macros de manière à éviter des problèmes
d'emboîtement ; pour des instructions la tournure idiomatique est
#define CHK_NULL(x) do { if ((x)==NULL) exit(1); }while(0)
En l'occurence, le test suspensif d'une condition est tellement commun
que le langage propose une macro-fonction toute faite à cet effet :
#inlude <assert.h>
#define CHK_NULL(x) assert(x != NULL)
En fait j'utiliserais même directement le assert() dans le code.


exit(-1);
Bouh... exit(3), voire un enum qui répertorie les différentes conditions
de sortie en cas d'erreur.


int main() {
char email[] = "fakessh@fakessh.eu";
char body[] = "From: "www"<fakessh@fakessh.eu>rn"
sendemail(email, body);
Je vais supposer que ces paramètres viendront de la ligne de commande
(dans le futur).
void sendemail(char *email, char *body) {
char *host_name = "smtp.fakessh.eu";
Mais là c'est une constante : remonte-la comme variable globale


char buf[1500] = {0};
Utilise des constantes de préprocesseur pour ce genre de valeurs-clé; au
passage cela permet d'expliciter les choix en commentaire:
/* au début du source */
#define TAILLE_TAMPON 1500 /* supérieur à 1000, RFC821 */

char buf[TAILLE_TAMPON] = {0};
memset(rbuf,0,TAILLE_TAMPON); /* cf. infra */
recv(sockfd, rbuf, TAILLE_TAMPON, 0);
/* etc. */


Le mélange entre l'ancien style (toutes les déclarations au début de la
fonction) et le nouveau style (les déclarations juste avant leur
utilisation, comme pour «timeout») n'est pas heureux.


//Define a timeout for resending data.
timeout.tv_sec = 2;
timeout.tv_usec = 0;
À quoi sert exactement ce commentaire ? Si on lit le code, on voit que
«timeout» n'est pas utilisé pour contrôler les appels send(2), seulement
la phase de connexion. Par ailleurs les contantes 2 et 0, ainsi que
l'argument 2 utilisé pour sleep(), doivent être des constantes
exactement comme le 1500 ci-dessus.


memset(rbuf,0,1500);
Utilise plutôt
memset(rbuf, 0, sizeof rbuf);


sockfd = open_socket((struct sockaddr *)&their_addr);
memset(rbuf, 0, 1500);
FD_ZERO(&readfds);
FD_SET(sockfd, &readfds);
retval = select(sockfd+1, &readfds, NULL, NULL, &timeout);
while(retval <= 0)
{
printf("reconnect...n");
sleep(2);
close(sockfd);
sockfd = open_socket((struct sockaddr *)&their_addr);
memset(rbuf,0,1500);
FD_ZERO(&readfds);
FD_SET(sockfd, &readfds);
retval = select(sockfd+1, &readfds,NULL,NULL,&timeout);
}
J'écrirais plutôt
for(essai=0; essai<MAX_ESSAIS, ++essai) {
sockfd = open_socket((struct sockaddr *)&their_addr);
memset(rbuf, 0, sizeof rbuf);
FD_ZERO(&readfds);
FD_SET(sockfd, &readfds);
retval = select(sockfd+1, &readfds,NULL,NULL,&timeout);
if( retval > 0 )
break;
printf("reconnecte...n");
close(sockfd); /*fermeture ressource dès que possible*/
sleep(DÉLAI_ATTENTE_NOUVEAU_CONNECT);
}


recv(sockfd, rbuf, 1500, 0);
printf("%sn", rbuf);
À partir de ce point, il n'y a plus de contrôle d'erreur, et c'est
inadmissible.
Il n'y pas non plus de contrôle du protocole SMTP. Je veux bien que ce
soit une ébauche, mais AVANT d'essayer d'utiliser SSL, tu devrais
d'abord programmer la logique du protocole de base, et vérifier que cela
fonctionne ; puis implémenter les délais d'attente ; et ensuite
seulement rajouter SSL par-dessus. Dans le cas contraire, cela ressemble
à un programme vite fait uniquement destiné à balancer le plus vite
possible de grandes quantités de courriers électroniques à travers un
serveur SMTP accessible uniquement en SSL.


EHLO localhost
Non conforme à la RFC, et sera rejeté par des serveurs actuels.


recv(sockfd, rbuf, 1500, 0);
printf("%sn", rbuf);
//
memset(buf,0, 1500);
sprintf(buf, "STARTTLSrn");
send(sockfd, buf, strlen(buf), 0);
Et si le serveur te balance plus de 1500 caractères en réponse à EHLO?
l'initialization SSL risque de devenir « intéressante »...


sprintf(buf,"fakessh");
On avait le paramètre placé comme variable chaîne «email» pour tester ;
puis le nom de serveur «host_name», ce qui était moins joli ; mais là,
mettre la valeur directement dans le code, c'est encore plus moche !


recv_line(ssl);
On récupère la réponse du serveur, et on ne regarde même pas ? Autrement
dit, tu n'as aucune considération pour les exploitants du serveur. S'il
n'y a pas de problèmes, personne ne va s'en apercevoir, pas plus toi que
les autres ; mais s'il y a un quelconque problème, le serveur va
immédiatement te mettre en liste noire en considérant, à juste titre,
que tes envois ne sont pas contrôlés ; et avec les systèmes de détection
de pourriels via les listes noires (RBL, spamhaus et compagnie), ton
serveur va être refusé partout en un rien de temps, à la vitesse de
l'internet...


Et on arrive à...
//send mail content - "rn.rn" is the end mark of content
sprintf(buf, "%srn.rn", body);
Raccourci saisissant du protocole SMTP. Mais c'est un reflet du reste.
Se compliquer la vie à faire un programme en C pour juste aligner les
étapes-clés du protocole sans rien coder ni rien contrôler, ce n'est pas
logique : utilise plutôt Perl ou Python !


Antoine