Menu
▾
▴
[Net-snmp-patches] [ net-snmp-Patches-3044888 ] Feature addition: Library crash on loading some MIBs
|
From: SourceForge.net <no...@so...> - 2010-10-06 11:50:35
|
Patches item #3044888, was opened at 2010-08-14 17:26 Message generated for change (Comment added) made by gaurav7 You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312694&aid=3044888&group_id=12694 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 4 Private: No Submitted By: Gaurav Chaturvedi (gaurav7) Assigned to: Nobody/Anonymous (nobody) Summary: Feature addition: Library crash on loading some MIBs Initial Comment: Hi, While reading and parsing the MIB files using net-snmp library, there is no kind of error code or message returned to the calling application. So, this leaves user in an illusion that all MIB files have been loaded successfully. In some cases (like MIB containing "TRAP-TYPE"), the library crashes for the first object. I have developed a feature to find the syntax error in MIB files, having "TRAP-TYPE" as one of the element and return the string containing detailed error and names of erroneous MIB modules. Changes made: A new parameter has been added to the structure "tree" (struct tree) named "parseErrorString". After calling the API "read_all_mibs()", the application can now retrieve the error code/string using pointer to the tree root (as returned by read_all_mibs" API). e.g. treeRoot->parseErrorString. It is the caller's responsibility to free the memory for "char *parseErrorString". Tested on: Windows XP 32-bit, Windows 2003 64-bit, Linux (Tsuse32 and 64-bits), Solaris 8 sparc, Solaris 8 sparc v9, Solaris 10 i386, Solaris 10 amd64 Patch over: Net-SNMP Version 5.4.1.2 Sample MIB (which causes the library to crash on parsing) and the changed files have been attached. Kindly consider the patch for official build and make me one of the proud contributers of this library. Please contact me at below mentioned contact points in case of any issue. Thanks Gaurav Chaturvedi (+91) 95794-43976 E-Mail ID: gau...@gm... ---------------------------------------------------------------------- Comment By: Gaurav Chaturvedi (gaurav7) Date: 2010-10-06 17:20 Message: Hi, Thanks for considering this feature. May I know what is the approximate time when this feature will be added in the major release. Kindly let me know in case any additional modification is required in order to make it suitable for including it in major release. Thanks Gaurav (+91) 9579443976 ---------------------------------------------------------------------- Comment By: Gaurav Chaturvedi (gaurav7) Date: 2010-08-17 10:49 Message: Hi, Thanks for your consideration. Please find my comments below: 1) We don't put coders names in comments in files. (eg, the "fix provided by ..." lines). The ChangeLog, CHANGES, and likes "NEWS" will have your name in it. >> As mentioned by you, I agree that names should not be part of code. You can remove them without any issues. They were just inserted as comments to simplify merging for you. 2) You use strcat, sprintf, etc. We've tried to avoid those functions and use the 'n' based functions instead (strncat, snprintf) because they're safer in general. Even if you know it'll fit, it's still wise to use them as many "code safety checking tools", which we run sometimes, marks them as "bad". >> strcat and sprintf are used because the parameters used are static arrays and hence are not harmful. They are completely safe to use in this case and I have tested the same on Windows, Linux and Solaris platforms (32-bits and 64-bits). So, I request you to include the code. 3) I haven't followed the malloc/free checking in it yet, but it looks like at least after you set the error string in the tree_head that the global should be NULLed. I worry about multiple calls to the function and what happens to the memory in the error string during those cases. >> The "parseErrorString" parameter is assigned the pointer "gpMibErrorString", which is freed before the next use, which is correct. Also, since it is used in "read_all_mibs" function which is called only once for most of the applications, there is no chance of memory being full/corrupted due to this. I request you to kindly consider the changes for official release. Please let me know in case of any issue or doubts. Regards Gaurav ---------------------------------------------------------------------- Comment By: Wes Hardaker (hardaker) Date: 2010-08-16 21:46 Message: Although I like the idea of the patch, I think it's going to need to wait until the next major release (5.7) to be considered. There are a few issues with it, some of which probably need wider discussion: 1) We don't put coders names in comments in files. (eg, the "fix provided by ..." lines). The ChangeLog, CHANGES, and likes "NEWS" will have your name in it. 2) You use strcat, sprintf, etc. We've tried to avoid those functions and use the 'n' based functions instead (strncat, snprintf) because they're safer in general. Even if you know it'll fit, it's still wise to use them as many "code safety checking tools", which we run sometimes, marks them as "bad". 3) I haven't followed the malloc/free checking in it yet, but it looks like at least after you set the error string in the tree_head that the global should be NULLed. I worry about multiple calls to the function and what happens to the memory in the error string during those cases. ---------------------------------------------------------------------- Comment By: Gaurav Chaturvedi (gaurav7) Date: 2010-08-16 12:04 Message: Hi, I am replacing the attached file (changed files) with "patch" files, so that it is easier to merge with the main branch. Thanks Gaurav ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312694&aid=3044888&group_id=12694 |