Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
130563010:
Invalid JSON output when BinaryValue suppressed. (Closed)
|
Created:
6 years, 10 months ago by Tom Sepez Modified:
6 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInvalid JSON output when BinaryValue type present as part of input object.
JSON doesn't have a syntax to support the base::BinaryValue type, so when these are present under a base::DictionarValue or base::ListValue, they must be suppressed. However, in doing so, the current code doesn't properly suppress the commas between members as part of the overall suppression.
Furthermore, if suppression was not enabled by the caller, we need to report failure if a BinaryValue is encountered.
BUG=340441
BUG=340733
R=thakis@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249830
Patch Set 1 #Patch Set 2 : Return an error when you try to write a binaryvalue. #Patch Set 3 : Better comments. #Patch Set 4 : Produce same strings as before when error is returned for those who don't check em. #
Total comments: 2
Patch Set 5 : Rebase. #
Total comments: 9
Patch Set 6 : Address comments. #Patch Set 7 : #ifdef out BinaryValue tests in debug mode to avoid crashes. #
Total comments: 2
Patch Set 8 : Log at ERROR level, not FATAL level. #
Total comments: 1
Patch Set 9 : Better naming. #Patch Set 10 : NOTREACHED() & return false at end of function. #
Messages
Total messages: 27 (0 generated)
Nico, please review or suggest an appropriate viewer. The modified test fails without the patch, since we get a stray leading comma in the string which is syntactically invalid.
On 2014/02/03 23:11:39, Tom Sepez wrote: > Nico, please review or suggest an appropriate viewer. The modified test fails > without the patch, since we get a stray leading comma in the string which is > syntactically invalid. +gab@, who has recently updated this as well.
https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.cc File base/json/json_writer.cc (right): https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.c... base/json/json_writer.cc:53: bool JSONWriter::BuildJSONString(const Value* const node, size_t depth) { Why return a bool? It seems the only way this can return false is if the recursive call (for list/dicts) returns false; but since the only way a leaf returns false is in the default case (which should never occur) the return value doesn't seem particularly useful to me. In fact since every Value::TYPE should be handled below I think we should remove the default: section (and clang will report a compile error if a new value type is added but not handled in the switch below). https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.c... base/json/json_writer.cc:59: case Value::TYPE_BOOLEAN: { I had also tried to fix the indent in my previous CL, but it makes the diff unreadable on reitveld... I thought I was the only that cared and left it as-is; seems this is no longer true, I suggest making a separate CL in which only the indentation is changed, then CLs actually changing behavior can have a cleaner diff.
On 2014/02/04 20:27:35, gab wrote: > https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.cc > File base/json/json_writer.cc (right): > > https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.c... > base/json/json_writer.cc:53: bool JSONWriter::BuildJSONString(const Value* const > node, size_t depth) { > Why return a bool? It seems the only way this can return false is if the > recursive call (for list/dicts) returns false; but since the only way a leaf > returns false is in the default case (which should never occur) the return value > doesn't seem particularly useful to me. > Not true. We can return false when a Binary value is encountered and we are not told to omit them. That's the whole point of the change. > In fact since every Value::TYPE should be handled below I think we should remove > the default: section (and clang will report a compile error if a new value type > is added but not handled in the switch below). > Fair enough. > https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.c... > base/json/json_writer.cc:59: case Value::TYPE_BOOLEAN: { > I had also tried to fix the indent in my previous CL, but it makes the diff > unreadable on reitveld... > > I thought I was the only that cared and left it as-is; seems this is no longer > true, I suggest making a separate CL in which only the indentation is changed, > then CLs actually changing behavior can have a cleaner diff. Fair enough, I'll quickly cobble together a prequel to first fix indentation. It bugs me :).
On 2014/02/04 20:31:44, Tom Sepez wrote: > On 2014/02/04 20:27:35, gab wrote: > > https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.cc > > File base/json/json_writer.cc (right): > > > > > https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.c... > > base/json/json_writer.cc:53: bool JSONWriter::BuildJSONString(const Value* > const > > node, size_t depth) { > > Why return a bool? It seems the only way this can return false is if the > > recursive call (for list/dicts) returns false; but since the only way a leaf > > returns false is in the default case (which should never occur) the return > value > > doesn't seem particularly useful to me. > > > Not true. We can return false when a Binary value is encountered and we are not > told to omit them. That's the whole point of the change. Ah I see, my bad, lost that in the crazy indent diff :)! > > > In fact since every Value::TYPE should be handled below I think we should > remove > > the default: section (and clang will report a compile error if a new value > type > > is added but not handled in the switch below). > > > Fair enough. > > > > https://codereview.chromium.org/130563010/diff/150001/base/json/json_writer.c... > > base/json/json_writer.cc:59: case Value::TYPE_BOOLEAN: { > > I had also tried to fix the indent in my previous CL, but it makes the diff > > unreadable on reitveld... > > > > I thought I was the only that cared and left it as-is; seems this is no longer > > true, I suggest making a separate CL in which only the indentation is changed, > > then CLs actually changing behavior can have a cleaner diff. > Fair enough, I'll quickly cobble together a prequel to first fix indentation. > It bugs me :).
Ok, please review the updated patch set. Thanks!
lgtm w/ comments Cheers! Gab https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.cc File base/json/json_writer.cc (right): https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.c... base/json/json_writer.cc:121: const ListValue* list; = NULL https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.c... base/json/json_writer.cc:154: const DictionaryValue* dict; = NULL https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.c... base/json/json_writer.cc:195: // Successful only if we're allowed to omit it. DLOG_IF(FATAL, !omit_binary_values_) << "..."; To keep the crash point as close to the failure point in Debug builds. https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.h File base/json/json_writer.h (right): https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.h... base/json/json_writer.h:52: // json_string_ will contain the JSON. "value is json_string_ will contain the JSON" I don't understand that... how about "|json_string_| will contain the JSON" ?
https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.cc File base/json/json_writer.cc (right): https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.c... base/json/json_writer.cc:121: const ListValue* list; On 2014/02/07 02:24:27, gab wrote: > = NULL Done. https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.c... base/json/json_writer.cc:154: const DictionaryValue* dict; On 2014/02/07 02:24:27, gab wrote: > = NULL Done. https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.c... base/json/json_writer.cc:195: // Successful only if we're allowed to omit it. On 2014/02/07 02:24:27, gab wrote: > DLOG_IF(FATAL, !omit_binary_values_) << "..."; > > To keep the crash point as close to the failure point in Debug builds. No. I don't want to crash here, instead I want to indicate failure back to the caller. Otherwise, the caller, when dealing with untrusted input from a renderer, has to walk the entire structure themselves to make sure there aren't any BinaryValues in it before continuing. https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.h File base/json/json_writer.h (right): https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.h... base/json/json_writer.h:52: // json_string_ will contain the JSON. On 2014/02/07 02:24:27, gab wrote: > "value is json_string_ will contain the JSON" > > I don't understand that... > > how about > > "|json_string_| will contain the JSON" > > ? Done.
https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.cc File base/json/json_writer.cc (right): https://codereview.chromium.org/130563010/diff/210001/base/json/json_writer.c... base/json/json_writer.cc:195: // Successful only if we're allowed to omit it. On 2014/02/07 02:30:50, Tom Sepez wrote: > On 2014/02/07 02:24:27, gab wrote: > > DLOG_IF(FATAL, !omit_binary_values_) << "..."; > > > > To keep the crash point as close to the failure point in Debug builds. > > No. I don't want to crash here, instead I want to indicate failure back to the > caller. Otherwise, the caller, when dealing with untrusted input from a > renderer, has to walk the entire structure themselves to make sure there aren't > any BinaryValues in it before continuing. But getting false in the caller doesn't tell you exactly what's wrong no? In Release builds (which all builds dealing with untrusted input are) you would get the behavior you desire. I don't see why a crash here in Debug builds is a bad thing. It helps developers new to this API who are using it in the wrong way see early what's wrong with their code.
don't see why a crash here in Debug builds is a bad thing. It helps developers > new to this API who are using it in the wrong way see early what's wrong with > their code. Fair enough. I'll put the DLOG_IF() in.
> Fair enough. I'll put the DLOG_IF() in. Done. See also the #ifdef in the unittest.
I changed my mind, you enlightened me with the ugly ifdef'ed tests, sorry about that. https://codereview.chromium.org/130563010/diff/360001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/130563010/diff/360001/base/json/json_writer_u... base/json/json_writer_unittest.cc:158: #ifdef NDEBUG This makes me realize that it's ugly to have to ifdef the tests for Debug mode. How about a DLOG_IF(ERROR) instead in json_writer.cc? This is what other base APIs do I think when faced with fatal failures (i.e. ERROR log + return false -- FATAL in low level APIs doesn't make sense after all).
https://codereview.chromium.org/130563010/diff/360001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/130563010/diff/360001/base/json/json_writer_u... base/json/json_writer_unittest.cc:158: #ifdef NDEBUG On 2014/02/07 03:12:12, gab wrote: > This makes me realize that it's ugly to have to ifdef the tests for Debug mode. > > How about a DLOG_IF(ERROR) instead in json_writer.cc? > > This is what other base APIs do I think when faced with fatal failures (i.e. > ERROR log + return false -- FATAL in low level APIs doesn't make sense after > all). Done.
@gab - thanks for all your help. Would you like to take a final look before I commit?
lgtm++, thanks, you'll still need Nico to stamp this Cheers, Gab
lgtm Please make the CL description less cryptic, I didn't understand what it was saying until I clicked through to the first bug. https://codereview.chromium.org/130563010/diff/50002/base/json/json_writer.cc File base/json/json_writer.cc (right): https://codereview.chromium.org/130563010/diff/50002/base/json/json_writer.cc... base/json/json_writer.cc:122: bool first_value_output = false; This can be parsed as both "is this the first value output" and "was the the first value output" which have opposite meanings. "is_first_value" would be unambiguous.
On 2014/02/07 18:17:05, Nico wrote: > lgtm > > Please make the CL description less cryptic, I didn't understand what it was > saying until I clicked through to the first bug. > Done. > https://codereview.chromium.org/130563010/diff/50002/base/json/json_writer.cc > File base/json/json_writer.cc (right): > > https://codereview.chromium.org/130563010/diff/50002/base/json/json_writer.cc... > base/json/json_writer.cc:122: bool first_value_output = false; > This can be parsed as both "is this the first value output" and "was the the > first value output" which have opposite meanings. "is_first_value" would be > unambiguous. How about |has_any_value_been_output| ?
On Fri, Feb 7, 2014 at 10:30 AM, <tsepez@chromium.org> wrote: > On 2014/02/07 18:17:05, Nico wrote: > >> lgtm >> > > Please make the CL description less cryptic, I didn't understand what it >> was >> saying until I clicked through to the first bug. >> > > Done. > > https://codereview.chromium.org/130563010/diff/50002/base/ >> json/json_writer.cc >> File base/json/json_writer.cc (right): >> > > > https://codereview.chromium.org/130563010/diff/50002/base/ > json/json_writer.cc#newcode122 > >> base/json/json_writer.cc:122: bool first_value_output = false; >> This can be parsed as both "is this the first value output" and "was the >> the >> first value output" which have opposite meanings. "is_first_value" would >> be >> unambiguous. >> > How about |has_any_value_been_output| ? > Works, but is kinda long. |wrote_value|? > > > https://codereview.chromium.org/130563010/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/07 18:30:10, Tom Sepez wrote: > On 2014/02/07 18:17:05, Nico wrote: > > lgtm > > > > Please make the CL description less cryptic, I didn't understand what it was > > saying until I clicked through to the first bug. > > > Done. > > > https://codereview.chromium.org/130563010/diff/50002/base/json/json_writer.cc > > File base/json/json_writer.cc (right): > > > > > https://codereview.chromium.org/130563010/diff/50002/base/json/json_writer.cc... > > base/json/json_writer.cc:122: bool first_value_output = false; > > This can be parsed as both "is this the first value output" and "was the the > > first value output" which have opposite meanings. "is_first_value" would be > > unambiguous. > How about |has_any_value_been_output| ? Settled on |first_value_has_been_output|.
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/130563010/490001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chromedriver_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/130563010/770001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/130563010/770001
Message was sent while issue was closed.
Change committed as 249830 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
