Summary:ASTERISK-08071: [patch] add checks for calloc calls
Reporter:Markus Elfring (elfring)Labels:
Date Opened:2006-11-06 08:36:19.000-0600Date Closed:2007-01-24 18:51:42.000-0600
Versions:Frequency of
Environment:Attachments:( 0) return_value_checking.diff
( 1) return_value_checking2.diff.gz
( 2) return_value_checking3.patch
Description:A follow-up for the topic "Check return codes everywhere" ...

Please complete the implementation for the function "set_var".
Comments:By: Russell Bryant (russell) 2006-11-06 12:05:41.000-0600

The missing check in check_expr.c has been fixed in revision 47232.  Thanks

By: Markus Elfring (elfring) 2006-11-07 04:00:33.000-0600

1. I assume that the added instruction "if (!t) return;" is not a completely accurate handling of a failed memory allocation.
Would a call like "exit(ENOMEM)" or "abort()" be more appropriate?

2. Would anybody like to scan the source files with static analysis tools like "SPlint"?
=> more examples for coding errors:
- http://svn.digium.com/view/asterisk/trunk/pbx/ael/ael.tab.c?rev=44377&view=markup:
 yyparse, ael_token_subst, npval

- http://svn.digium.com/view/asterisk/trunk/pbx/ael/ael_lex.c?rev=41527&view=markup:

- http://svn.digium.com/view/asterisk/trunk/apps/app_mixmonitor.c?rev=44379&view=markup:

- http://svn.digium.com/view/asterisk/trunk/main/ast_expr2f.c?rev=40722&view=markup:

- menuselect.c:

How can so many important return values be ignored?

By: Russell Bryant (russell) 2006-11-07 21:29:07.000-0600

No, I do not think exit() or abort() would be more appropriate.  My reasoning has already been explained on the asterisk-dev mailing list.

I will look at the other parts of the code you pointed out.

By: Russell Bryant (russell) 2006-11-07 21:37:18.000-0600

You need to be more specific when pointing out "coding errors" other than just saying "there is a coding error in this function".  Also, if you find errors, why not go ahead and create a patch to address them?  Also, as mentioned multiple times before, this is not a forum for asking questions.  For asking questions, please use the asterisk-dev mailing list.  Thanks

By: Markus Elfring (elfring) 2006-11-09 09:06:02.000-0600

I have assumed that a code maintainer will be quicker to see a fix than me.

Is the appended change suggestion useful?

By: Serge Vecher (serge-v) 2006-11-10 12:10:59.000-0600

elfring: haven't I asked you nicely to please follow the directions of bug-marshalls in 8181? I did. So why are you reopening this report when Russell has clearly instructed you not to do that and open a new report if you find additional bugs?

1. Please get a discaimer on file. See bottom of http://bugs.digium.com/main_page.php
2. Review http://www.asterisk.org/developers/Patch_Howto to find out how to make a proper patch.
3. Open a new report indicating your disclaimer status and attaching the proper patch.
4. You may also reopen 8160 and idicate your disclaimer you status when you are done with item 1.


By: Markus Elfring (elfring) 2006-11-18 14:26:36.000-0600

Did my letter arrive that I sent on Monday?

By: Anthony LaMantia (alamantia) 2006-11-20 18:25:45.000-0600

kevin fleming who managers most of the disclaimer status here is on vacation for thanksgiving right now and wont be back till next week.. i've added an alert for this issue in my calander for nextweek when he will be back and i can check the status of your disclaimer.

By: Anthony LaMantia (alamantia) 2006-11-28 13:58:27.000-0600

elfring, per our guidelines you need to provide us with an svn diff aginst the current release of asterisk you are patching.

see: http://www.asterisk.org/developers/Patch_Howto
as well as our guidelines on bugs.digium.com

By: Markus Elfring (elfring) 2006-11-29 16:12:05.000-0600

Another try ...

Is the appended variant acceptable now?

By the way, I have read on the page "http://www.digium.com/bugguidelines.html" that the format "diff -u" would also be appropriate. Well, I assumed that the package on the download page was recent enough corresponding to the item "4. Use latest code".

By: Anthony LaMantia (alamantia) 2006-11-30 17:16:24.000-0600

ah, please update your patchs as flat text files, so we can all view them easily in our web-browsers.. i've uploaded the text version of your latest patch for review.

By: Anthony LaMantia (alamantia) 2006-11-30 21:13:09.000-0600

I am working on fixing up your patch a tad to keep our coding guide lines
you can view it here
if you need to make any more changes please make them off this branch.

By: Markus Elfring (elfring) 2006-12-01 13:26:32.000-0600

I am interested in feedback on your opinion that my suggestion "exit(ENOMEM)" is the wrong reaction.

Is it expected that any memory allocation will still succeed for your prefered call "ast_log(LOG_WARNING, ...)"?

By: Anthony LaMantia (alamantia) 2006-12-01 18:42:32.000-0600

using exit() is really the wrong way to go for this issue at least.

By: Markus Elfring (elfring) 2006-12-02 11:30:37.000-0600

What is your reason for this view?

How often would you like to retry a failed memory allocation?

I guess that it is unlikely to achieve a graceful exit.

By: Anthony LaMantia (alamantia) 2007-01-18 13:23:46.000-0600

it's more of a personal opinion/feeling that exiting if a calloc() fails callin exit() would be the wrong thing to do.

By: Tilghman Lesher (tilghman) 2007-01-18 15:06:51.000-0600

If memory allocation fails, you may abort the call.  However, you may NOT abort the process.  Doing so affects more than just the single call you are processing.  If the administrator feels that the process should be taken down, then the administrator may do that.  However, the process is not permitted to make that decision based solely on a failed memory allocation.

This is policy.

By: Markus Elfring (elfring) 2007-01-22 08:12:44.000-0600

How are you sure that the process can be kept correctly running after a failed memory allocation?
Do you hope that required resources are only temporarily missing?

By: Tilghman Lesher (tilghman) 2007-01-22 09:11:36.000-0600

This is not the forum for policy changes.  If you wish to challenge a policy, you may do so on the asterisk-dev list.

By: Russell Bryant (russell) 2007-01-24 18:51:42.000-0600

I'm closing this out since this patch is not something we are willing to accept.