How to submit the patches?

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?

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,

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);
......

-----------------------------------------
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.

============================
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 + }

---------------------
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 }

Thanks!
Tianyin

On Fri, Dec 28, 2012 at 10:26 PM, Tianyin Xu <tixu_at_cs.ucsd.edu> wrote:
> Hi all,
>
> I would like to improve the configuration parsing and checking of
> Squid. I was/am involved in a project which uses Squid to do testing
> under different configurations.
>
> During the project, I found quite a number of problems related to
> configuration design (I hope you do think they are problems).
>
> For example,
> http://bugs.squid-cache.org/show_bug.cgi?id=3729
> which is an integer overflow bug.
>
> But, to me, there're more problems than this particular bug, for
> example, when parsing the numeric values, we have such code:
>
> /* src/Parsing.cc */
> 93 i = strtoll(token, NULL, 10);
>
> which does not check integer overflow nor bad numeric values (e.g.,
> introduced by typos).
>
> In GetInteger(void), we use
>
> /* src/Parsing.cc */
> 108 if (sscanf(token, "%i", &i) != 1)
>
> like atoi(), sscanf is also unsafe and has no way to check whether the
> number has integer overflow or bad characters, etc. It's better to use
> strtoll() for string to integer convention which has the ability to
> check users' misconfigurations (strtoll can also deal with octal and
> hex numbers with prefix (0 and 0x)).
>
> The consequence of these is the current system accepts the following
> misconfigurations silently without notifying users:
>
> http_port 6553M5
> fqdncache_size 3500000000
>
> In the example above, the system listens to port 6553 with the fqdn
> cache size of -794967296.
>
> Another example is like parse_onoff() in "src/cache_cf.cc"
>
> 2559 if (!strcasecmp(token, "on") || !strcasecmp(token, "enable"))
> 2560 *var = 1;
> 2561 else
> 2562 *var = 0;
>
> what if the user misconfigures like "yes" or "true", or even a typo
> like "enabe"?
>
> These problems are definitely not bugs but I think good configuration
> design with good checking and parsing can prevent a lot of latter
> problems, and can significantly save users' time. After all, not every
> users even administrators are reading our source code. So I hope I can
> make our software more user-friendly and popular.
>
> How do you guys think? If you guys think it's a good idea. I'm willing
> to spend time on it.
>
> Best regards,
> Tianyin
>
>
> --
> Tianyin XU,
> http://cseweb.ucsd.edu/~tixu/

--
Tianyin XU,
http://cseweb.ucsd.edu/~tixu/