[FFmpeg-devel] [RFC] SDP Generation

[Luca Abeni]

Hi Michael,

thanks for the comments, and sorry again for the errors in my patch.

Michael Niedermayer wrote:
[...]
>>     const char *name;
>>     const char *info;
>> };
> 
> please document the fields of structures

Ok. I am working on doxygenning the code, and I'll include all the 
comments in the next patch.


>> 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?

My reasoning was: if I fill the whole buffer, sdp_print() will be called 
with size = 0, so it will return NULL, and next call to sdp_print() will 
  have buff = NULL. This if is to avoid repeating the check after every 
call to sdp_print(). But this reasoning is wrong, because I 
misunderstood the snprintf() return value (see below).


[...]
>>     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 ?

If size == 0 (I expected this to happen because of my misunderstanding 
of the return value) or in case of error (I just wanted to consider the 
case in which vsnprintf() fails for some reason).


[...]
> 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

Opss. I was under the impression that vsnprintf() printed at most n 
characters, and returned the number of written characters. Sorry about 
that, I'll fix the code going for an strlcatf()-like solution as you 
suggest.


> [...]
>> 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

I think I did:
- "; config=" is 9 characters
- extradata is encoded in extradata_size * 2 characters
- then there is the \0 at the end
Am I missing something?


[...]
> 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;

Ok.

Thanks again for the review; I appreciate it!


			Thanks,
				Luca