Re: How to submit the patches?

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
Received on Fri Jan 04 2013 - 03:39:05 MST