Hi, Amos, Could you tell me how to get the newest version, i.e., 3.HEAD / trunk (as you mentioned)?
I followed the bazaar instructions on the wiki (http://wiki.squid-cache.org/BzrInstructions)\ $ bzr checkout http://bzr.squid-cache.org/bzr/squid3/trunk trunk but the trunk I checked out is pretty much the stable version (i.e., squid-3.2.5) I modified. For example, there's no compat/xato.h and xato.cc files as you mentioned in the previous email. Also. xstro.cc and Parsing.cc is almost the same as the old ones. So I guess the version is not the newest. I plan to modify a bit on the newest version to make it easy for you to merge. (According to my understanding, I should submit the patches on the squid-dev, am I right?) Thanks a lot! Tianyin On Thu, Jan 3, 2013 at 7:38 PM, Amos Jeffries <[email protected]> wrote: > On 2013-01-04 14:50, Tianyin Xu wrote: >> >> Hi, all, >> >> As proposed earlier, I was working on improving the configuration >> checking and parsing during the holiday. >> >> I mainly focused on two aspects including numeric parsing and enum >> values by using more strict checkings and more verbosity. I hope to >> make the software more user-friendly and less error-prone :-) >> Since all the code are used during user configuration translation, it >> wouldn't introduce too much overhead. >> >> Could anyone tell me how to submit my patches? >> > > Our merge procedure and requirements are all documented here: > http://wiki.squid-cache.org/MergeProcedure > > > >> I describe what I did as follows, and I hope it makes sense to you guys. >> >> ================ >> 1. Numeric Options >> ================ >> -------------------------------- >> 1.1. Integer Overflow >> -------------------------------- >> >> I mainly modified the parsing functions in squid-3.2.5/src/Parsing.cc >> and squid-3.2.5/compat/xstrto.cc, > > > FYI: changes built on stable branches such as 3.2 are most likely to have > problems merging. We will be applying your changes to the 3.HEAD / trunk and > differences between the branches can cause issues both code bugs on broken > patching and design problems as the feature differences build up (ie your > early patches will be in trunk but not 3.2, so your later patches against > 3.2 will clash with your early ones). > > >> >> There're several places have integer overflow problems, e.g., >> >> 240 int >> 241 xatoi(const char *token) >> 242 { >> 243 - return xatol(token); >> 244 + long long input; >> 245 + int ret; >> 246 + >> 247 + input = xatoll(token, 10); >> 248 + ret = (int) input; >> 249 + >> 250 + if (input != (long long) ret) { >> 251 + debugs(0, 0, "ERROR: The value '" << token << "' is >> larger than the type 'int'."); >> 252 + self_destruct(); >> 253 + } >> 254 + >> 255 + return ret; >> 256 +} >> >> As this patch illustrated, I mainly check the converted values to make >> sure it's valid in range. >> >> ------------------------------------------------ >> 1.2. String to Number conversion >> ------------------------------------------------ >> >> Here, I mainly abandon the direct use of "atoi()", "sscanf()", etc.. >> Squid has already noticed these problem (that's why we have our own >> xatoi, xatos, etc), so what I do is to make them consistent with what >> we are expecting. Moreover, some configuration parsing functions still >> use unsafe code, though we've already had the safe conversion >> functions. So I simply let it use the available conversion. The >> following patch is one of the typical modifications: >> >> 328 GetInteger64(void) >> ...... >> 338 - i = strtoll(token, NULL, 10); >> 339 + input = xatoll(token, 10); >> ...... >> > > We have a compatibility library where these xato*() functions should be > defined. The versions spread out in the parsing code need to be moved to > compat/xato.h and compat/xato.cc files. Then we need to provide wrapper > replacements so the main code can use the "normal" ato*() names and the > compiler replace with our improved xato*() at build time. > > When replacing unsafe functions please ensure the bug in the unsafe one is > documented as the reason in the commit message. And where relevant please > add definitions in compat/unsafe.h (with documentation why please) to make > compile impossible when they are used. > > >> ----------------------------------------- >> 1.3. Signedness conversion >> ----------------------------------------- >> >> For signedness issues, I mainly check the patterns which directly >> assigns a signed value to an unsigned variable, as follows: >> >> 165 - s->tcp_keepalive.idle = atoi(t); >> 166 + s->tcp_keepalive.idle = xatoui(t); >> >> In this example, "idle" is defined as "unsigned int" and "t" comes >> from user input. A negative value (i.e., "-1" to disable the feature) >> will be converted to a super large positive value if we use atoi() >> directly. > > > 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. > > >> >> ============================ >> 2. Enum Options (including Boolean) >> ============================ >> ---------------------- >> 2.1. Strictness >> ---------------------- >> >> For enum options, I mainly strict the if-else checking. I don't know >> whether this's appropriate. >> >> 93 if (!strcasecmp(token, "on") || !strcasecmp(token, "enable")) >> 94 *var = 1; >> 95 - else >> 96 + else if(!strcasecmp(token, "off") || !strcasecmp(token, >> "disable")) >> 97 *var = 0; >> 98 + else { >> 99 + debugs(0, 0, "ERROR: Invalid option -- Boolean options >> only accept 'on'/'off' or 'enable'/'disable' to turn on/off the >> feature."); >> 100 + self_destruct(); >> 101 + } >> > > It is much like what I would have done. Thank you. Just two notes: > > * The debug should probably be: debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), > ... > > * we generally document on/off as the values for boolean, enable/disable are > just also-supported values. You can drop enable/disable out of the debugs > help documentation and probably add them as extra cases with debugs > informing the admin to update their config to use values on/off. > > >> --------------------- >> 2.2. Verbosity >> --------------------- >> >> I personally like to give more error logs when users configuration >> input has something wrong. For example, in parse_access_log(), if none >> of the keyword can match the user's input, the following message will >> be printed out: >> >> debugs(3, 0, "Log format '" << logdef_name << "' is not defined"); >> >> So, I add several log message like this one: >> >> 133 - else >> 134 + else { >> 135 + debugs(0, 0, "ERROR: Invalid option " << token << ": >> 'uri_whitespace' accepts 'strip', 'deny', 'allow', 'encode', and >> 'chop'."); >> 136 self_destruct(); >> 137 + } >> 138 } >> > > These are fine, but please add the extra verbose debug details with the > level parameter being DBG_PARSE_NOTE(2) [or maybe something deeper than 2], > so that they show up with squid -k parse and not on regular reconfigure > operations. > > > Amos -- Tianyin XU, http://cseweb.ucsd.edu/~tixu/
