Summary:	ASTERISK-08071: [patch] add checks for calloc calls
Reporter:	Markus Elfring (elfring)	Labels:
Date Opened:	2006-11-06 08:36:19.000-0600	Date Closed:	2007-01-24 18:51:42.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
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" ... http://bugs.digium.com/view.php?id=8157 Example: Please complete the implementation for the function "set_var". http://svn.digium.com/view/asterisk/trunk/utils/check_expr.c?rev=9991&view=markup
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:  ael2_parse - http://svn.digium.com/view/asterisk/trunk/apps/app_mixmonitor.c?rev=44379&view=markup:  launch_monitor_thread - http://svn.digium.com/view/asterisk/trunk/main/ast_expr2f.c?rev=40722&view=markup:  SET_STRING, SET_NUMERIC_STRING - menuselect.c:  process_deps 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. Thanks 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 http://svn.digium.com/view/asterisk/teamanthonyl/8295-calloc 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. Example: http://svn.digium.com/view/asterisk/team/anthonyl/8295-calloc/apps/app_mixmonitor.c?r1=48171&r2=48172 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.