Re: Some suggested changes to gwlib MIME module
Alexander Malysh <[email protected]>
| Newsgroups | gmane.comp.mobile.kannel.devel |
|---|---|
| Message-ID | <[email protected]> |
Hi,
the patch looks good. +1 for committing it.
Thanks,
Alex
P.S. Paul please use [PATCH] in subject next time.
Stipe Tolj schrieb:
> Paul Bagyenda wrote:
>
>> Hi,
>> Mbuni MMSC/MMS VAS Gateway makes heavy use of Kannel's GWLIB,
>> which has resulted partly in some patches to Kannel from our side, to
>> take care of missing stuff (or stuff we thought looked faulty). Many
>> of these are now merged into CVS.
>> One area we've been talking about for a while now is the Kannel
>> MIME module. With Stipe's help we've managed to narrow down our issues
>> to one problem: The fact that the layout of the MIMEEntity structure
>> is exposed in the header file. The potential problems are subtle. In
>> our case we tripped on the following problem: If you have a multi-part
>> MIME object that you have constructed manually (as is done for
>> instance in the Mbuni code that converts an MMS into an MM7/SOAP
>> message), and you want to extract its header (mime_entity_headers())
>> and then its body (mime_entity_body()), you end up with a wrong or
>> missing boundary element for the content type in the header. The
>> result is that header/body can no longer be combined and parsed!
>> In discussions with Stipe, it looks like whichever way you cut it,
>> this kind of problem would arise, as long as users can muck around
>> with the internal structure of a MIMEEntity. Solution: Hide it and
>> add additional helper functions for manipulating the structure.
>> Suggested solution attached as a patch against CVS.
>
> Thanks a lot Paul,
>
> This looks good. I have revised the patch (attached) in terms of
> CodingStyle and some slight logic change.
>
> Can you please review and comment if this is still what we want?
>
> Comments from others please... I'm +1 on commiting the suggersted
> solution to cvs in order to solve the direct access problematic of the
> MIMEEntity object.
>
> Stipe
>
> -------------------------------------------------------------------
> Kölner Landstrasse 419
> 40589 Düsseldorf, NRW, Germany
>
> tolj.org system architecture Kannel Software Foundation (KSF)
> http://www.tolj.org/ http://www.kannel.org/
>
> mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org
> -------------------------------------------------------------------
>
>
> ------------------------------------------------------------------------
>
> ### Eclipse Workspace Patch 1.0
> #P gateway-cvs
> Index: gwlib/mime.c
> ===================================================================
> RCS file: /home/cvs/gateway/gwlib/mime.c,v
> retrieving revision 1.12
> diff -u -r1.12 mime.c
> --- gwlib/mime.c 9 Dec 2005 02:41:14 -0000 1.12
> +++ gwlib/mime.c 6 Feb 2006 01:15:35 -0000
> @@ -75,6 +75,18 @@
>
>
> /********************************************************************
> + * Internal data structure types
> + */
> +
> +struct MIMEEntity {
> + List *headers;
> + List *multiparts;
> + Octstr *body;
> + struct MIMEEntity *start; /* in case multipart/related */
> +};
> +
> +
> +/********************************************************************
> * Creation and destruction routines.
> */
>
> @@ -150,20 +162,61 @@
> }
>
>
> +/*
> + * This function checks that there is a boundary parameter in the headers
> + * for a multipart MIME object. If not, it is inserted and passed back to caller
> + * in the variable boundary_elem.
> + */
> +static void fix_boundary_element(List *headers, Octstr **boundary_elem)
> +{
> + Octstr *value, *boundary;
> +
> + if (headers == NULL)
> + return;
> +
> + /*
> + * Check if we have an boundary parameter already in the
> + * Content-Type header. If no, add one, otherwise parse which one
> + * we should use.
> + * XXX this can be abstracted as function in gwlib/http.[ch].
> + */
> + value = http_header_value(headers, octstr_imm("Content-Type"));
> + boundary = http_get_header_parameter(value, octstr_imm("boundary"));
> + if (boundary == NULL) {
> + boundary = octstr_format("_boundary_%d_%ld_%c_%c_bd%d",
> + random(), (long)time(NULL), 'A' + (random()%26),
> + 'a'+(random() % 26), random());
> + octstr_format_append(value, "; boundary=%S", boundary);
> +
> + http_header_remove_all(headers, "Content-Type");
> + http_header_add(headers, "Content-Type", octstr_get_cstr(value));
> + http_header_add(headers, "MIME-Version", "1.0");
> + }
> + octstr_destroy(value);
> +
> + /* if we got a NULL'ed value, throw away, otherwise return */
> + if (boundary_elem)
> + *boundary_elem = boundary;
> + else
> + octstr_destroy(boundary);
> +}
> +
> +
> /********************************************************************
> * Mapping function from other data types, mainly Octstr and HTTP.
> */
>
> static Octstr *mime_entity_to_octstr_real(MIMEEntity *m, unsigned int level)
> {
> - Octstr *mime, *value, *boundary;
> + Octstr *mime, *boundary;
> List *headers;
> long i;
>
> gw_assert(m != NULL && m->headers != NULL);
>
> + boundary = NULL;
> mime = octstr_create("");
> -
> +
> /*
> * First of all check if we have further MIME entity dependencies,
> * which means we have further MIMEEntities in our m->multiparts
> @@ -181,26 +234,9 @@
> goto finished;
> }
>
> - /*
> - * Check if we have an boundary parameter already in the
> - * Content-Type header. If no, add one, otherwise parse which one
> - * we should use.
> - * XXX this can be abstracted as function in gwlib/http.[ch].
> - */
> + /* This call ensures boundary exists, and returns it */
> + fix_boundary_element(m->headers, &boundary);
> headers = http_header_duplicate(m->headers);
> - value = http_header_value(headers, octstr_imm("Content-Type"));
> - boundary = http_get_header_parameter(value, octstr_imm("boundary"));
> - if (boundary == NULL) {
> - boundary = octstr_format("_boundary_%d_%ld_%c_%c_bd%d",
> - random(), (long)time(NULL), 'A' + (random()%26),
> - 'a'+(random() % 26), random());
> - octstr_format_append(value, "; boundary=%S", boundary);
> -
> - http_header_remove_all(headers, "Content-Type");
> - http_header_add(headers, "Content-Type", octstr_get_cstr(value));
> - http_header_add(headers, "MIME-Version", "1.0");
> - }
> - octstr_destroy(value);
>
> /* headers */
> for (i = 0; i < gwlist_len(headers); i++) {
> @@ -229,8 +265,10 @@
>
> /* add the last boundary statement, but hive an EOL
> * if we are on the top of the recursion stack. */
> +
> /* if (level > 0) */
> - octstr_append(mime, octstr_imm("\r\n"));
> + octstr_append(mime, octstr_imm("\r\n"));
> +
> octstr_append(mime, octstr_imm("\r\n--"));
> octstr_append(mime, boundary);
> octstr_append(mime, octstr_imm("--\r\n"));
> @@ -238,7 +276,6 @@
> octstr_destroy(boundary);
>
> finished:
> -
> return mime;
> }
>
> @@ -409,6 +446,9 @@
>
> gw_assert(m != NULL && m->headers != NULL);
>
> + /* Need a fixup before hand over. */
> + fix_boundary_element(m->headers, NULL);
> +
> headers = http_header_duplicate(m->headers);
>
> return headers;
> @@ -423,6 +463,10 @@
>
> gw_assert(m != NULL && m->headers != NULL);
>
> + /* For non-multipart, return body directly. */
> + if (mime_entity_num_parts(m) == 0)
> + return octstr_duplicate(m->body);
> +
> os = mime_entity_to_octstr(m);
> context = parse_context_create(os);
> e = mime_entity_create();
> @@ -447,6 +491,108 @@
> }
>
>
> +MIMEEntity *mime_entity_duplicate(MIMEEntity *e)
> +{
> + MIMEEntity *copy = mime_entity_create();
> + int i, n;
> +
> + mime_replace_headers(copy, e->headers);
> + copy->body = e->body ? octstr_duplicate(e->body) : NULL;
> +
> + for (i = 0, n = gwlist_len(e->multiparts); i < n; i++)
> + gwlist_append(copy->multiparts,
> + mime_entity_duplicate(gwlist_get(e->multiparts, i)));
> + return copy;
> +}
> +
> +
> +void mime_replace_headers(MIMEEntity *e, List *headers)
> +{
> + gw_assert(e != NULL);
> + gw_assert(headers != NULL);
> +
> + http_destroy_headers(e->headers);
> + e->headers = http_header_duplicate(headers);
> +}
> +
> +
> +int mime_entity_num_parts(MIMEEntity *e)
> +{
> + gw_assert(e != NULL);
> +
> + return (e->multiparts ? gwlist_len(e->multiparts) : 0);
> +}
> +
> +
> +/*
> + * Append a new part to list of body parts, copy is added.
> + * Note that if it was not multipart, this action makes it so!
> + */
> +void mime_entity_add_part(MIMEEntity *e, MIMEEntity *part)
> +{
> + gw_assert(e != NULL);
> + gw_assert(part != NULL);
> +
> + gwlist_append(e->multiparts, mime_entity_duplicate(part));
> +}
> +
> +
> +MIMEEntity *mime_entity_get_part(MIMEEntity *e, int i)
> +{
> + MIMEEntity *m;
> + gw_assert(e != NULL);
> + gw_assert(i >= 0);
> + gw_assert(i < gwlist_len(e->multiparts));
> +
> + m = gwlist_get(e->multiparts, i);
> + gw_assert(m);
> + return mime_entity_duplicate(m);
> +}
> +
> +
> +void mime_entity_remove_part(MIMEEntity *e, int i)
> +{
> + MIMEEntity *m;
> +
> + gw_assert(e != NULL);
> + gw_assert(i >= 0);
> + gw_assert(i < gwlist_len(e->multiparts));
> +
> +
> + m = gwlist_get(e->multiparts, i);
> + gwlist_delete(e->multiparts, i, 1);
> +
> + mime_entity_destroy(m);
> +}
> +
> +
> +void mime_entity_replace_part(MIMEEntity *e, int i, MIMEEntity *newpart)
> +{
> +
> + MIMEEntity *m;
> +
> + gw_assert(e != NULL);
> + gw_assert(i >= 0);
> + gw_assert(i < gwlist_len(e->multiparts));
> +
> + m = gwlist_get(e->multiparts, i);
> + gwlist_delete(e->multiparts, i, 1);
> + gwlist_insert(e->multiparts, i, mime_entity_duplicate(newpart));
> +
> + mime_entity_destroy(m);
> +}
> +
> +
> +void mime_entity_set_body(MIMEEntity *e, Octstr *body)
> +{
> + gw_assert(e != NULL);
> + gw_assert(body != NULL);
> +
> + octstr_destroy(e->body);
> + e->body = octstr_duplicate(body);
> +}
> +
> +
> /********************************************************************
> * Routines for debugging purposes.
> */
> Index: gwlib/mime.h
> ===================================================================
> RCS file: /home/cvs/gateway/gwlib/mime.h,v
> retrieving revision 1.8
> diff -u -r1.8 mime.h
> --- gwlib/mime.h 11 Feb 2005 15:35:48 -0000 1.8
> +++ gwlib/mime.h 6 Feb 2006 01:15:35 -0000
> @@ -90,21 +90,54 @@
>
> #include "gwlib/gwlib.h"
>
> -
> -/* Define an generic MIME entity structure to be
> - * used for MIME multipart parsing. */
> -typedef struct MIMEEntity {
> - List *headers;
> - List *multiparts;
> - Octstr *body;
> - struct MIMEEntity *start; /* in case multipart/related */
> -} MIMEEntity;
> +typedef struct MIMEEntity MIMEEntity;
>
>
> /* create and destroy MIME multipart entity */
> MIMEEntity *mime_entity_create(void);
> void mime_entity_destroy(MIMEEntity *e);
>
> +/*
> + * Make a copy of a MIME object.
> + */
> +MIMEEntity *mime_entity_duplicate(MIMEEntity *e);
> +
> +/*
> + * Replace top-level MIME headers: Old ones removed completetly
> + */
> +void mime_replace_headers(MIMEEntity *e, List *headers);
> +
> +/*
> + * Get number of body parts. Returns 0 if this is not
> + * a multipart object.
> + */
> +int mime_entity_num_parts(MIMEEntity *e);
> +
> +/*
> + * Append a new part to list of body parts. Copy is added.
> + */
> +void mime_entity_add_part(MIMEEntity *e, MIMEEntity *part);
> +
> +/*
> + * Get part i in list of body parts. Copy is returned.
> + */
> +MIMEEntity *mime_entity_get_part(MIMEEntity *e, int i);
> +
> +/*
> + * Replace part i in list of body parts. Old one will be deleted.
> + */
> +void mime_entity_replace_part(MIMEEntity *e, int i, MIMEEntity *newpart);
> +
> +/*
> + * Remove part i in list of body parts.
> + */
> +void mime_entity_remove_part(MIMEEntity *e, int i);
> +
> +/*
> + * Change body element of non-multipart entity.
> + */
> +void mime_entity_set_body(MIMEEntity *e, Octstr *body);
> +
> /*
> * Parse the given Octstr that contains a normative text representation
> * of the MIME multipart document into our representation strucuture.