Haven't seen a PR, so went ahead and implemented as discussed:
https://github.com/c-ares/c-ares/commit/d0f7d5ee
On 3/12/18 9:23 AM, Francisco Sedano Crippa (fsedanoc) wrote:
Hi folks.
Yes, ares_set_servers_csv() returned what it should. It’s just the behavior afterwards which is not correct. I’ll
submit PR for adding this check, thanks.
*. .:|:.:|:. _Francisco Sedano_ | CCIE 14859, Tech Lead Software Engineering | CSG Enterprise Access and Services
Group (EASG)*
*From: *c-ares <c-ares-boun...@cool.haxx.se> on behalf of David Drysdale
<drysd...@google.com>
*Reply-To: *c-ares hacking <c-ares@cool.haxx.se>
*Date: *Sunday, 11 March 2018 at 08:29
*To: *c-ares hacking <c-ares@cool.haxx.se>
*Subject: *Re: Wrong handling on badly formatted strings passed to
set_servers_csv
On Sat, Mar 10, 2018 at 1:03 PM, Brad House
<b...@mainstreetsoftworks.com<mailto:b...@mainstreetsoftworks.com>> wrote:
Did ares_set_servers_csv() return ARES_EBADSTR as it should?
There's a test that implies that it will:
https://github.com/c-ares/c-ares/blob/master/test/ares-test-misc.cc#L86-L88
It does appear when it does that, it leaves the channel in a "bad" state
since existing servers are cleared before
parse. I'm not sure of any other instances where a channel might have no
servers, as even in the case where it
can't determine one from configuration, it uses a localhost fallback. It
may make more sense to kill the
ares__destroy_servers_state() call at the beginning of set_servers_csv(),
since ares_set_servers_ports() clears
it which gets called after a successful parse.
I'd agree that there should probably be a sanity check in ares_send() to
not try an invalid ares_malloc(),
ARES_ESERVFAIL would be as good an error condition as any in this case.
+1
-Brad
On 3/9/18 7:07 PM, Francisco Sedano Crippa (fsedanoc) wrote:
Hello,
I noticed today if you pass a string with spaces to set_servers_csv,
like:
"127.0.0.1 , 200.0.0.1"
It will take the first server as "127.0.0.1 " (note the space), it will
notice it's not a valid IP and fail.
So far so good.
However, nservers for the channel will stay set to -1, so when
ares_send is called, this will be executed:
query->server_info = ares_malloc(channel->nservers *
sizeof(query->server_info[0]));
The negative value will be misinterpreted to a huge number since
argument is size_t and we agree things smell
really bad from here. In practice, such a mem allocation fails and we
return ENOMEM (which is also
misleading), but it's a very incorrect behaviour.
I was thinking on just adding a check at the beginning of ares_send() to
exit if nservers is <= 0.
Do you guys agree with the approach? If that’s the case, which error do
you suggest to return? No one really
matches, I’d say ARES_ENOTFOUND, but that implies we tried to contact
the server…
Thanks!
*. .:|:.:|:. _Francisco Sedano_ | CCIE 14859, Tech Lead Software
Engineering | CSG Enterprise Access
and Services Group (EASG)*
