New issue
Advanced search Search tips

Issue 860181 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Dec 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocked on:
issue 724678



Sign in to add a comment

Port latest base/json code to libchrome

Project Member Reported by ljusten@chromium.org, Jul 4

Issue description

The latest version of base/json from Chromium contains important security hardening work and should be merged into libchrome.
 
Labels: -Pri-1 Pri-2
Triage nag: This Chrome OS bug has an owner but no component. Please add a component so that this can be tracked by the relevant team.
<UI triage> Bug owners, please add the appropriate component to your bug. Thanks!
Blockedon: 724678
Components: Enterprise
Adding Enterprise component since JSON parsing is used for policy, but the parser is also used in many other places like ARC and buffet.

Btw, the CL for this has been uploaded a while ago, but it's blocked on a libchrome uprev, which basically makes the CL unreviewable.

https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1125732

Cc: emaxx@chromium.org
Re comment 5:

I think other people returned to backporting various features to our old libchrome, e.g. http://crrev.com/c/1322429, http://crrev.com/c/1194709. Because the timeline of the libchrome uprev seems to be still unclear (despite the huge amount of work already done in the last few months).

So seems like it should be reasonable to unblock your CL?
I can give it another shot. The addition of Optional definitely makes the patch a lot smaller. From Hidehiko's comment on  crbug.com/724678  it sounds like it's almost done modulo CQ snags, so hopefully it'll be done soon. It's a huge task for sure!
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/6c7d1bc8525a04f2b98a83c65fe8ca23c8685dad

commit 6c7d1bc8525a04f2b98a83c65fe8ca23c8685dad
Author: Lutz Justen <ljusten@chromium.org>
Date: Sat Dec 08 19:23:22 2018

Libchrome: Add support for C++14

Switches the SConstruct file over to compile with C++14 in order to
remove the delta to upstream. In particular, this allows us to use
std::make_unique instead of base::MakeUnique.

BUG= chromium:860181 
TEST=Compiled libchrome

Change-Id: Ice6721752a6569dd14913ce16a69e2fd5d50f6bc
Reviewed-on: https://chromium-review.googlesource.com/1358591
Commit-Ready: Lutz Justen <ljusten@chromium.org>
Tested-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[modify] https://crrev.com/6c7d1bc8525a04f2b98a83c65fe8ca23c8685dad/chromeos-base/libchrome/libchrome-456626.ebuild
[add] https://crrev.com/6c7d1bc8525a04f2b98a83c65fe8ca23c8685dad/chromeos-base/libchrome/files/libchrome-456626-Support-C++14.patch

Labels: M-73
Labels: -Pri-2 Pri-1
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6

commit 18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6
Author: Lutz Justen <ljusten@chromium.org>
Date: Tue Dec 18 16:27:10 2018

libchrome: Uprev base/json to r576297

This CL uprevs to base/json version to r576297 to catch important
security hardening work.

It is done this way instead of checking the code directly into the
libchrome repo so it doesn't interfere with upcoming changes to
libchrome. It is not done as a regular patch to simplify code review.

r576297 was picked since it is planned to be the next uprev target of
libchrome, so it's going to be easy to handle (basically, just revert
this CL). Moreover, it contains all hardening CLs except one
(CL:1308196).

To simplify code review,
- PS1 contains vanilla r576297 code and
- PS2+ contains the changes to make it compile with libchrome again.

There were some minor changes to make the API backwards compatible:
- Adding back JSON_DETACHABLE_CHILDREN (does not do anything, but some
  Chrome OS code still uses the flag).
- const StringPiece& json instead of StringPiece in ReadAndReturnError.
Note that when recompiling dependencies, the build system does not pick
up incremental changes from a libchrome-456626-rN.ebuild ->
libchrome-456626-rN+1.ebuild uprev.

BUG= chromium:860181 
TEST=Manually ran all unit tests and fuzzer tests.

Change-Id: I79529fc297c0b62cb9100bedb840c894b7851354
Reviewed-on: https://chromium-review.googlesource.com/1349354
Commit-Ready: Lutz Justen <ljusten@chromium.org>
Tested-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>

[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_value_serializer_unittest.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_value_converter.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_parser_unittest.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/string_escape.cc
[rename] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/libchrome-456626-r6.ebuild
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_reader_unittest.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/string_escape_unittest.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_file_value_serializer.h
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_writer_unittest.cc
[modify] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/libchrome-456626.ebuild
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/string_escape_fuzzer.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_perftest.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_writer.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/OWNERS
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_writer.h
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_reader.h
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_correctness_fuzzer.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_string_value_serializer.h
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_file_value_serializer.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_reader_fuzzer.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_parser.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/string_escape.h
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_string_value_serializer.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_value_converter.h
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_parser.h
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_value_converter_unittest.cc
[add] https://crrev.com/18c8a23d498dafc8dc3b8e6e3adc7bc3973aa9f6/chromeos-base/libchrome/files/base_json_based_on_r576297/json_reader.cc

Status: Fixed (was: Started)
Hi Lutz, does this bug require any manual verification by the Enterprise test team? If yes, could you please provide some steps? Thanks!
Status: Verified (was: Fixed)
assuming unit tests are sufficient here
Sorry, missed this. This shouldn't require manual testing, just keep an eye open for JSON failures in different components (e.g. authpolicy). I ran all unit tests and fuzzers manually.

Note that libchrome still doesn't seem to be running unit tests unless I'm missing something. Its BUILD.gn file doesn't compile any unittest.cc files.

Sign in to add a comment