Hi, Amos, I addressed your audit results one by one (see my reply below each audit entry). The updated patches is attached.
Two things I want to mention in the very beginning: 1. I checked the version according to your information, and made sure it's the newest one (revision 12566). :-) 2. For the precedence level, you mentioned "m*d/u == "m*(d/u)". I checked both C and C++ reference manual*,**. * M. A. Ellis and B. Stroustrup, "The Annotated C++ Reference Manual", Addison-Wesley Publishing, Sept. 1999. ** S. P. Harbison and G. L. Steele Jr., "C: A Reference Manual (3rd Edition)", Prentice Hall, 1991. Both of them claims that multiplicative operators (include *, /, %) have the same precedence level, and group left-to-right. So I think it makes sense to rewrite m*d/u*2 to (m*d/u)*2 but not to m*(d/u)*2. Let me know if I misunderstood. Further audit or comments are very welcomed. Thanks a lot! Tianyin On Fri, Jan 4, 2013 at 10:12 PM, Amos Jeffries <[email protected]> wrote: > On 5/01/2013 3:05 p.m., Tianyin Xu wrote: >> >> Hi, all, >> >> I modified my patches according to Amos's comments and suggestions >> (Thanks a lot, Amos!). >> >> The attached patch includes all the modifications upon 3.HEAD/trunk. >> >> The only thing I didn't follow is to move the xato* functions in the >> compatibility library and to replace all the ato* functions in the >> code. I thought about it for quite a while, but decided not to have a >> xato* series as lib calls. The main reason is that these xato* >> functions are tailored for configuration parsing instead of general >> string-to-number functionality. All the current ato* functions (in >> src/Parsing.cc) will call self_destruct() when error occurs, which in >> turn prints out the bungled line in the configuration file (this's >> really good!). > > > Getting rid of that bungled message is one of our config parsing upgrade > goals. If we can adjust the xato*() calls to report errors without aborting > Squid it will make them more useful and re-usable. They should still log an > error, but IMO aborting is going too far. The code which calls them also has > more context to decide whether the config option deserves a fatal() exit or > not. You can see a good example of that in src/HelperChildConfig.cc at the > second chunk where you change atoi() to xatoi(); the calling code is > reporting what values are valid, why and then calling self_destruct() > anyway. > > >> On the other hand, strto* series as lib calls already >> provide safe string-to-number conversion, and anyone can take >> advantage of them. So I don't think there's any need to have a new >> series of lib calls. >> >> I modified some of the ato* functions during configuration file >> parsing using the safe functions we implemented, but didn't change >> others because they might not be used in configuration parsing (at >> least I don't have good understandings of them). My modifications are >> type safe, i.e., they didn't introduce any semantic or format change. >> Backward compatibility is most important for users and customers. For >> example, I use xatoui() only when the type of the dest variable is >> "unsigned int"; GetPercentage() is introduced to support options like >> "20%" (previously atoi ignores %), etc. >> >> Amos proposed the great idea that "In the cases where -1 is used we >> want to update the documentation and parser to accept the string >> "none". Which maps internally to the disabled value (usually still -1 >> or -2). After which anything else must be >0." But for now, I don't >> want a tremendous change in my patch because it's quite dangerous to >> abandon all the previous "-1"-like settings. > > > Ack. Do this one separately as its own patch please. > > > Now, audit results from reading your patches... > > compat/xstrto.cc: > * the first chunk changing strtoul() to strtol() then checking for <0 is > incorrect. Values such as 0xffffffff (with 8 f's) which strtol() maps to > negative numbers and are valid unsigned numbers. In order to use signed > conversion function to produce a unsigned value the output of the signed > function MUST have a type larger than the unsigned functions output (ie > 64-bit signed conversion function can be used to parse 32-bit unsigned > values, and cannot be used to parse 64-bit signed values). > The more correct change would be strtoul() -> strtoll(). However, these > netfilter authored functions have been placed through some strict unit tests > by the netfilter guys. They may have overlooked one case you can identify, > but before you change them please add cppunit tests to verify that all the > input cases are still handled correctly (we currently have none for these > functions). Yes, I changed it back. > > * the second chunk. does it still compile when you use the C++ > static_cast<unsigned long>() operator ? > Yes, it still compiles so I changed it using static_cast, and use static_cast in other functions. > > in src/Parsing.cc: > * the debugs message about "expected characters" is not very helpful. It > should be changed to say what values are expected and what was found. > For example: debugs(..., "ERROR: '-' is not a digit. '"<< token << "' is > supposed to be a number."); > Yes, I refined the log messages. > * I spot another bug which remains after your fixes. The "end" pointers > passed to strto*() functions is documented as being NULL or checked for NULL > value, but the Squid code is not initializing any of the "char *end;" > variables to NULL. Please fix these as part of your patch. "char *end = > NULL;" should be sufficient. > Yes, I initialized all the end pointers to NULL. > * in xatoi(). This is C++ code please define the variable at point of first > use when possible. Also, please use int64_t/uint64_t types rather than "long > long". > For example: int64_t input = xatoll(token, 10); > The same goes for other functions where you are adding new local variables. > Yes, I change all the "long long" to "int64_t". > * in xatoui(). The 64-bit signed conversion function xatol() or xatoll() > needs to be used, not the xatoi() one. > Yes, I changed xatoi() to xatoll(). > * GetInteger() has the requirement of parsing masks. Please retain the > comment about that. The portion about %i is invalid after changing to > xatoll(), but GetInteger() still has that usage requirement and your patch > only works because our xatoll() actually uses strtoll() as the backend. > Yes, I recover the previous comment and modified it. > * GetPercentage() can make use of the strto*() function min/max values to > double-check the range accepted. > Also, please document in Parsing.h that 0%-100% range is validated and > what happens when the value is out of range. > Yes, I added the document in Parsing.h > in src/cache_cf.cc: > * please do not add useless empty lines. Such as in the first chunk at line > 1017 between the if statement '}' and function ending '}'. > Yes, I deleted that empty line and checked others. > * the next two chunks doing "m*d/u*2" please add () brackets around the > (d/u) portion to clarify what the formula is calculating. Sorry I still do not understand this part. Please see the explanation at the beginning of this mail. > > * the debugs statements "will be deprecated in future releases." is > incorrect. The situation is "is deprecated." from the moment these debugs > lines get merged. Yes, I changed the log. > > * at line 3475. port is being set by the strtol() call in the if() > condition. There is no need to parse the token twice. > I kept it. > * the debugs you are adding at line 3705 appears to already be present in my > copy of trunk with better text. > Hmm. I'm wondering if you did get trunk checkout properly now. > I checked the newest version and that one is not there.:-) > in src/wccp2.cc: > * the second chunk is parsing a port value. It should probably be using > xatos() and p defined as a uint16_t. > I kept for the better logging. > > > Question: can you identify what the benefits/costs of using the strto*() > versus the ato*() functions family are? I think now is a good time to > identify which has the best performance and safety to migrate the code to > them. xstrtoui() / xatoui() are overlapping, as are some of the others. > > > Amos -- Tianyin XU, http://cseweb.ucsd.edu/~tixu/
config.patch
Description: Binary data
