[FFmpeg-devel] [RFC] SDP Generation

[Michael Niedermayer]

Hi

On Thu, Jul 12, 2007 at 09:44:46AM +0200, Luca Abeni wrote:
> Hi all,
> 
> I finally had some time to work on my SDP generator again, and here is a 
> new version, which hopefully incorporates all the feedback I received.
> 
> Changes since last version:
> 1) fixed the broken digit_to_char() implementation
> 2) switched from url_fprintf to snprintf(). To avoid code repetition, I 
> introduced an sdp_print() helper function (better names for it are 
> welcome ;-)
> 3) removed attribute_write(), and merged some print()s
> 4) changed
> 	int is_multicast;
>         is_multicast = ...
>    into
>         int is_multicast = ...
> 5) merged the for() cycles in avf_sdp_create()
> 
> The only comment I did not follow is the removal of dest_write(): since 
> - it is called in two different points during the SDP generations
> - it must print something only if dest_addr != NULL
> - the format of the printed string depends on the TTL value
> I think that merging dest_write() in an sdp_print() would result in more 
> complex and less readable code... I hope that this version is acceptable.
> 
> 
> I tested this SDP generator for some time (I have patches for using it 
> in ffmpeg.c and ffserver.c... I'll send them once sdp.c is in), and it 
> seems to work reasonably well. My impression is that sdp.c could be 
> acceptable for svn (I think the three FIXMEs are not important, adn can 
> be fixed later). So, I added a license header (copied from avformat.h), 
> and I am sending it again.
> 
> 
> 				Thanks,
> 					Luca

[...]
> struct sdp_session_level {
>     int sdp_version;
>     int id;
>     int version;
>     int start_time;
>     int end_time;
>     int ttl;
>     const char *user;
>     const char *src_addr;
>     const char *dst_addr;
>     const char *name;
>     const char *info;
> };

please document the fields of structures


> 
> static char *sdp_print(char *buff, int size, const char *fmt, ...)
> {
>     va_list vl;
>     int res;
> 
>     if (buff == NULL) {
>         return NULL;
>     }

how can this be true?


> 
>     va_start(vl, fmt);
>     res = vsnprintf(buff, size, fmt, vl);
>     va_end(vl);
>     if (res <= 0) {
>         return NULL;
>     }

how exactly is vsnprintf() supposed to return <=0 ?
errors in the fmt string are fatal internal errors and should be dealt with
abort()/assert(0) if we do check for them


> 
>     return buff + res;

argh, the whole code is a overcomplicated exploitable mess

more precissely vsnprintf() will stop writing at the end of the buffer
but the pointer you return will not be limited to the buffer size and
is then used to calculate the remaining size which is negative but unsigned
thus killing all checks in subsequent calls

why not write a strlcatf() that is a strlcat which adds a printf style 
formatted string?
no need to mess with the buf ptr or remaining buf size ...


[...]
> static char *sdp_media_attributes(char *buff, int size, AVCodecContext *c, int payload_type)
> {
>     char *config = NULL;
> 
>     switch (c->codec_id) {
>         case CODEC_ID_MPEG4:
>             if (c->flags & CODEC_FLAG_GLOBAL_HEADER) {
>                 config = av_malloc(10 + c->extradata_size * 2);
>                 if (config == NULL) {
>                     av_log(NULL, AV_LOG_ERROR, "Cannot allocate memory for the config info\n");

please check that 10 + c->extradata_size * 2 doesnt overflow


[...]
>     switch (c->codec_type) {
>         case CODEC_TYPE_VIDEO:
>             type = "video";
>             break;
>         case CODEC_TYPE_AUDIO:
>             type = "audio";
>             break;
>         case CODEC_TYPE_SUBTITLE:
>             type = "text";
>             break;
>         default:
>             type = "application";
>             break;
>     }

case CODEC_TYPE_VIDEO   : type = "video"      ; break;
case CODEC_TYPE_AUDIO   : type = "audio"      ; break;
case CODEC_TYPE_SUBTITLE: type = "text"       ; break;
default                 : type = "application"; break;



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070717/f9738bc6/attachment.pgp>