Opened 8 years ago

Closed 8 years ago

#1626 closed defect (fixed)

Error: Unable to parse response from Auth: Expecting , delimiter: line 1 column 87 (char 87)

Reported by: jreed Owned by: jelte
Priority: medium Milestone: Sprint-20120403
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

/Auth/listen_on> config set [0]/port 5300
/Auth> config commit
Error: Unable to parse response from Auth: Expecting , delimiter: line 1 column 87 (char 87)
Configuration not committed

Logging:

2012-01-24 09:52:00.612 INFO  [b10-boss.boss] BIND10_SOCKET_GET requesting socket [::]:5300 of type TCP from the creator
2012-01-24 09:52:00.614 INFO  [b10-boss.boss] BIND10_SOCKET_CREATED successfully created socket 20
2012-01-24 09:52:00.636 INFO  [b10-boss.boss] BIND10_SOCKET_GET requesting socket [::]:5300 of type UDP from the creator
2012-01-24 09:52:00.636 INFO  [b10-boss.boss] BIND10_SOCKET_CREATED successfully created socket 22
2012-01-24 09:52:00.640 INFO  [b10-boss.boss] BIND10_SOCKET_GET requesting socket [0.0.0.0]:53 of type TCP from the creator
2012-01-24 09:52:00.641 ERROR [b10-boss.boss] BIND10_SOCKET_ERROR error on bind call in the creator: 13/Permission denied
2012-01-24 09:52:00.643 ERROR [b10-auth.server_common] SRVCOMM_ADDRESS_FAIL failed to listen on addresses (Error response when requesting socket: "Error creating socket on bind")
2012-01-24 09:52:00.647 ERROR [b10-auth.auth] AUTH_CONFIG_UPDATE_FAIL update of configuration failed: Server configuration failed: Error response when requesting socket: "Error creating socket on bind"
2012-01-24 09:52:00.648 ERROR [b10-cfgmgr.cfgmgr] CFGMGR_BAD_UPDATE_RESPONSE_FROM_MODULE Unable to parse response from module Auth: Expecting , delimiter: line 1 column 87 (char 87)
2012-01-24 09:52:00.650 ERROR [b10-cmdctl.cmdctl] CMDCTL_COMMAND_ERROR error in command set_config to module ConfigManager: Unable to parse response from Auth: Expecting , delimiter: line 1 column 87 (char 87)

This was caused by not having privilege to set the port for the other default listen_on.

My complaint is that the error message in bindctl is not useful to understand the issue.

Subtickets

Change History (22)

comment:1 follow-up: Changed 8 years ago by jreed

Now I received a related error:

/Auth/datasources[0]/zones> config go [0]
/Auth/datasources[0]/zones[0]> config set origin foo.
/Auth/datasources[0]/zones[0]> config set file "/home/reed/tmp/J.zone"
/Auth/datasources[0]/zones[0]> config commit
Error: Unable to parse response from Auth: Expecting , delimiter: line 1 column 142 (char 142)
Configuration not committed
/Auth/datasources[0]/zones[0]> config go /
> config diff
{'Auth': {'datasources': [{'zones': [{'origin': 'foo.', 'file': '/home/reed/tmp/J.zone'}], 'type': 'memory'}]}}

What is the problem this time?

comment:2 in reply to: ↑ 1 Changed 8 years ago by jreed

Replying to jreed:

In that case, it was using a zone reproducing problem in ticket #696. So #696 has different result.

jelte says auth is sending malformed responses back to cfgmgr.

comment:3 Changed 8 years ago by jelte

Yes, tt's responding with malformed data (nested quotes):

{ "result": [ 1, "Server configuration failed: Error response when requesting socket: "Error creating socket on bind"" ] }

I suspect the best fix would be to go all the way down to the StringElement? object and do in-place escaping when creating a StringElement?:

diff --git a/src/lib/cc/data.cc b/src/lib/cc/data.cc
index 77f948a..bdb4cb4 100644
--- a/src/lib/cc/data.cc
+++ b/src/lib/cc/data.cc
@@ -25,6 +25,7 @@
 #include <sstream>
 
 #include <boost/algorithm/string.hpp> // for iequals
+#include <boost/algorithm/string/replace.hpp>
 
 #include <cmath>
 
@@ -195,6 +196,10 @@ bool operator!=(const Element& a, const Element& b) {
 //
 // factory functions
 //
+StringElement::StringElement(const std::string& v) : Element(string), s(v) {
+    boost::algorithm::replace_all(s, "\"", "\\\"");
+}
+
 ElementPtr
 Element::create() {
     return (ElementPtr(new NullElement()));
diff --git a/src/lib/cc/data.h b/src/lib/cc/data.h
index 5c731e6..a8b4356 100644
--- a/src/lib/cc/data.h
+++ b/src/lib/cc/data.h
@@ -424,7 +424,7 @@ class StringElement : public Element {
     std::string s;
 
 public:
-    StringElement(std::string v) : Element(string), s(v) {};
+    StringElement(const std::string& v);
     std::string stringValue() const { return (s); }
     using Element::getValue;
     bool getValue(std::string& t) { t = s; return (true); }

Should at least fix this issue, there may be a more efficient way to implement it (and we should probably escape at least the single quote as well). And perhaps it should be done when converting to JSON, not really sure atm. I'm also not sure whether the python side needs something similar too.

Of course you'd still get an error, but at least it would be the real one (i.e. error creating socket).

comment:4 Changed 8 years ago by jelte

note that the diff isn't directly applicable; it solves this specific problem, but does not do escaping correctly.

comment:5 Changed 8 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:6 follow-up: Changed 8 years ago by jreed

Should this be in review? (patch was provided)

comment:7 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:8 in reply to: ↑ 6 Changed 8 years ago by jelte

Replying to jreed:

Should this be in review? (patch was provided)

not really, the patch is incomplete (it solves this specific issue but introduces another one)

comment:9 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:10 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120403

comment:11 Changed 8 years ago by jinmei

It's not clear to me what's the current status of this ticket.

Is it that Jelte is expected to complete it? (If so I think it should
be assigned to him).

comment:12 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

picking this up, i have reconsidered the approach above, and have concluded that the escaping should not be in construction, but in the creation of the JSON data

comment:13 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

So it was a missed part of the JSON conversion, really, '"' must be escaped, as does a number of other chars (\, \n, \t etc). AFAICT some implementations also escape the forward slash, so the parser code allows it, but we don't do so ourselves (I have tried out the python implementation, it accepts both too)

With proper escaping of strings in JSON output, the quoted error message comes across correctly.

comment:14 Changed 8 years ago by muks

  • Owner changed from UnAssigned to jelte

My review:

+        if (c == '\\') {
+            // next char must be either another \ or "
+            // see the spec for allowed escape characters
+            if (strchr("\"\\/\b\f\n\r\t", in.peek()) != NULL) {

Shouldn't that be "\"
/bfnrt"? Also, it seems they should be decoded into the correct escape characters here (\b, \f, \n, \r and \t).

You should also add testcases for this, or am I missing them from some previous commits? :)

comment:15 Changed 8 years ago by jelte

  • Owner changed from jelte to muks

ah doh, of course :)

made switches out of them. Testcases are in data_unittests.cc (escapeHelper and the test below it), or did you mean different tests?

comment:16 Changed 8 years ago by muks

Hi jelte

The escaping in str_from_stringstream() now looks OK. I've added a few more testscases to the branch, some of which fail. Please look at these:

    // Can't have escaped quotes outside strings                                                                                  
    EXPECT_THROW(Element::fromJSON("\\\"\\\""), JSONError);
    // Inside strings is OK                                                                                                       
    EXPECT_NO_THROW(Element::fromJSON("\"\\\"\\\"\""));
    // String not terminated                                                                                                      
    EXPECT_THROW(Element::fromJSON("\"hello"), JSONError);
    // Bad string                                                                                                                 
    EXPECT_THROW(Element::fromJSON("hello\"foobar\""), JSONError);
    // A whitespace test                                                                                                          
    EXPECT_NO_THROW(Element::fromJSON("  \n  \r \t  \n \n    \t"));

comment:17 Changed 8 years ago by muks

  • Owner changed from muks to jelte

comment:18 Changed 8 years ago by jelte

  • Owner changed from jelte to muks

ack, there was no explicit check to see if strings were terminated (the \"hello test), added.

The last one failed because it was not enclosed in double quotes, so it does fail because not all whitespace was skipped there (added \b and \r), but it would still fail because it is essentially empty data. I enclosed it in doublequotes, and I also added one more test to make sure all whitespace-type 'outside' of actual element data is skipped (with a fix and some extra tests).

comment:19 Changed 8 years ago by muks

  • Owner changed from muks to jelte

Sorry for bringing it back to you once again. I've updated the tests:

  • \f is not handled as whitespace
  • Element::fromJSON("\"foobar\"hello") throws nothing

With these, I think now it's done.

comment:20 Changed 8 years ago by jelte

  • Owner changed from jelte to muks

hehe, we're venturing well out of scope here, but good catch :)

The reason extra data was allowed is that all was based on streams; but of course checking for 'end' if the input is a string is not much of a problem; even found some bad input in a completely unrelated python test :)

Also replaced all those direct lists of characters with one WHITESPACE const.

BTW, I also moved a few of these test cases; some don't have much to do with escaping, but with general 'whitespace' handling in conversion, so i've moved those to the relevant tests.

comment:21 Changed 8 years ago by muks

  • Owner changed from muks to jelte

Looks good now :)

comment:22 Changed 8 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

thanks, merged, closing ticket

Note: See TracTickets for help on using tickets.