Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(601)

Issue 868253006: Replace usage of md5 with sha256 for generation of mojo app ids. (Closed)

Created:
5 years, 10 months ago by eseidel
Modified:
5 years, 10 months ago
Reviewers:
qsr, abarth-chromium
CC:
abarth-chromium, Aaron Boodman, ben+mojo_chromium.org, darin (slow to review), esprehn, mojo-reviews_chromium.org, ojan, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
git@github.com:domokit/mojo.git@master
Target Ref:
refs/heads/master
Project:
mojo
Visibility:
Public.

Description

Replace usage of md5 with sha256 for generation of mojo app ids. Before we start adding more uses of mojo app ids we should use to a non-broken hash function. :) What was holding me back before was I wasn't aware of us having an incremental hash api other than base/md5.h (others in base only operate on the full input data), however it turns out that the crypto/ library has one in crypto/secure_hash.h. R=abarth@chromium.org BUG= Committed: https://chromium.googlesource.com/external/mojo/+/ca9659ccbfe3311a40970ae4aa11e8364e3257b3

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -12 lines) Patch
M shell/dynamic_application_loader.cc View 2 chunks +13 lines, -10 lines 1 comment Download
M sky/tools/mojo_cache_linker.py View 3 chunks +9 lines, -2 lines 0 comments Download
M sky/tools/skydb View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (1 generated)
eseidel
5 years, 10 months ago (2015-01-29 20:56:24 UTC) #1
abarth-chromium
lgtm
5 years, 10 months ago (2015-01-29 20:59:14 UTC) #2
eseidel
Committed patchset #1 (id:1) manually as ca9659ccbfe3311a40970ae4aa11e8364e3257b3 (presubmit successful).
5 years, 10 months ago (2015-01-29 20:59:54 UTC) #3
qsr
5 years, 10 months ago (2015-01-30 15:15:59 UTC) #5
Message was sent while issue was closed.
https://codereview.chromium.org/868253006/diff/1/shell/dynamic_application_lo...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/868253006/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:317: // Currently we use sha256 from
crypto/secure_hash.h
Eric: there is a few things to consider for this:
1) Only hash a few k of the library, that should be enough to get a hash, and
will be quicker to compute and need less I/O.
2) Try to compute the has while we get the byte from the network in case of a
network load, so that we do not have to re-read the same bytes
3) Fix the current race -> if the same application is in 2 different location on
the network, and we load those at the same time, there is a race.

Powered by Google App Engine
This is Rietveld 408576698