[asterisk-dev] [Code Review] 3734: media formats: Add a 'none' format to prevent NULL format derefences; update unit tests

[Matt Jordan]

> On July 9, 2014, 9:45 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/main/bridge_channel.c, line 324
> > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62646#file62646line324>
> >
> >     Check still needed or can we just assert?

This one is a bit interesting, in that the formats on the channel get saved off, an attempt to make the channel compatible with everything else occurs - where new formats are placed on the channel - leading to this path getting hit.

I have mixed feelings about this: there's a lot going on leading up to this point, and I'm not sure we can always predict that there will never be a case where a format won't be NULL. We can try asserts and restore these checks if needed.


> On July 9, 2014, 9:45 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/main/codec_builtin.c, line 118
> > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62649#file62649line118>
> >
> >     I'm not sure AUDIO is appropriate.  Maybe UNKNOWN is better, or even a new AST_MEDIA_TYPE_NULL?

If this isn't audio, we're going to get more NULL format hits.

A lot of things assume that there is a single audio format on the channel; consider every single format that is passed to the translation core. Whether or not that's appropriate is another question; but if the point of this is to try and prevent seg faults from a channel with no formats getting abused, not using audio is going to lead to crashing.


> On July 9, 2014, 9:45 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/main/format_cache.c, line 437
> > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62650#file62650line437>
> >
> >     This procedure could previously unset a format:
> >     ast_format_cache_set("ulaw", NULL);
> >     
> >     Is that not needed?
> >     
> >     Also ast_format_get_name is called many times, it would be better to call it once and save to char *name;

It isn't needed.

The name field is actually broken: you can set a format by some random name, but that name is not cached with a format; hence, you cannot look up a format using the name you provided.

Instead, you need to use ast_format_create_named. That will allow you to assign a custom name to format, which is equivalent to what this used to do.

Caching the result of ast_format_get_name is unlikely to be useful; compilers tend to handle simple accessors just fine.


> On July 9, 2014, 9:45 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/main/format_cache.c, line 442
> > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62650#file62650line442>
> >
> >     I think just assert for !format.

No, this safety check is useful. It prevents someone from creating a named empty format and attempting to cache it.

Making an API robust - particularly when those checks do not sit in a critical path - is not a bad thing :-)


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3734/#review12535
-----------------------------------------------------------


On July 9, 2014, 7:37 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3734/
> -----------------------------------------------------------
> 
> (Updated July 9, 2014, 7:37 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch does two things:
> 
> * It updates a few of the unit tests for some of the API changes. In particular, it focuses on adding some tests for formats with attributes and their expected behaviour. A few other non-format related unit tests were updated as well to handle off nominals detected during testing.
> 
> * It adds an 'ast_format_none' format. This format is a dummy format that can be used instead of a NULL pointer to prevent having to put NULL dereference checks into every place in the codebase. Channels are no assigned this format immediately upon creation, and their default capabilities are set to have it. As this format's codec has no translation (nor a representation in the RTP engine), it _shouldn't_ cause harm.
> 
> * A few NULL checks were put in anyway into key areas in a few modules. These were ones that were hit hard by the unit tests and prone to crashing if presented a NULL format.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c 418259 
>   /team/group/media_formats-reviewed-trunk/tests/test_format_cache.c 418259 
>   /team/group/media_formats-reviewed-trunk/tests/test_core_format.c 418259 
>   /team/group/media_formats-reviewed-trunk/tests/test_cel.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/format_cap.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/format_cache.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/codec_builtin.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/codec.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/channel.c 418259 
>   /team/group/media_formats-reviewed-trunk/main/bridge_channel.c 418259 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/format_cache.h 418259 
>   /team/group/media_formats-reviewed-trunk/include/asterisk/codec.h 418259 
> 
> Diff: https://reviewboard.asterisk.org/r/3734/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> There is a FRACK on shutdown, but it doesn't appear to be caused by this patch (things didn't run long enough without this patch before)
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140710/2c8186a9/attachment.html>