Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1321253011:
Remove dependency on //crypto in //ipc, except on windows. (Closed)
|
Created:
5 years, 3 months ago by Sergey Ulanov Modified:
5 years, 3 months ago Reviewers:
Tom Sepez CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@fallback_clean Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove dependency on //crypto in //ipc, except on windows.
//ipc was depending on //crypto just for crypto::RandBytes(), but
after crrev.com/347312 that function is used only on Windows, so
the //crypto is needed only on windows.
R=tsepez@chromium.org
Committed: https://crrev.com/384a29c919f7111c4f50ae9ae56e1599b5faa50a
Cr-Commit-Position: refs/heads/master@{#348638}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 66 (31 generated)
sergeyu@chromium.org changed reviewers: + tsepez@chromium.org
This should unblock this CL https://codereview.chromium.org/1312463005/ which was unintentionally pulling boringssl into NaCl IRT through ipc and crypto.
Is there a corresponding GYP change?
On 2015/09/08 15:51:16, Tom Sepez wrote: > Is there a corresponding GYP change? In the second patch set updated GYP as well.
lgtm
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1321253011/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1321253011/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:100001) has been deleted
Tom, PTAL: there was issue with "gn check" that doesn't handle conditional includes correctly, so I moved nonce generation to handle_attachment_win.cc.
Patchset #8 (id:160001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
lgtm
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1321253011/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/240001
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1321253011/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321253011/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321253011/260001
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/384a29c919f7111c4f50ae9ae56e1599b5faa50a Cr-Commit-Position: refs/heads/master@{#348638}
Message was sent while issue was closed.
Committed patchset #10 (id:260001) manually as 384a29c919f7111c4f50ae9ae56e1599b5faa50a (presubmit successful).
Message was sent while issue was closed.
On 2015/09/14 17:13:35, Sergey Ulanov wrote: > Committed patchset #10 (id:260001) manually as > 384a29c919f7111c4f50ae9ae56e1599b5faa50a (presubmit successful). In the future, I'd appreciate it if you cc-ed me on changes that affect code I've written in the last two weeks. 1) Your CL removes SerializeToBuffer(), despite making no mention of it in the CL description. I'm trying to land a CL right now that depends on the existence of that function. 2) In the not-so-distant future, Mac will also depend on crypto::RandBytes. 3) It's not immediately clear to me why you had to move the location of the code that uses crypto::RandBytes - why couldn't you have just updated the gyp files?
Message was sent while issue was closed.
On Mon, Sep 14, 2015 at 1:53 PM, <erikchen@chromium.org> wrote: > On 2015/09/14 17:13:35, Sergey Ulanov wrote: > >> Committed patchset #10 (id:260001) manually as >> 384a29c919f7111c4f50ae9ae56e1599b5faa50a (presubmit successful). >> > > In the future, I'd appreciate it if you cc-ed me on changes that affect > code > I've written in the last two weeks. > I'm sorry about it. Initially this CL was intended to be trivial dependency removal, without any code changes so I sent it to Tom because he is an owner in ipc. But then I couldn't land that simple version because gn check was failing (see below), so I had to make other changes, but failed to include you. BTW you can add yourself in the watchlist (see src/WATCHLIST) if you'd like to be notified about changes in the same directory. > > 1) Your CL removes SerializeToBuffer(), despite making no mention of it in > the > CL description. I'm trying to land a CL right now that depends on the > existence > of that function. > 1. That function wasn't used anywhere, i.e. it was dead code. 2. It didn't look like that function is useful anyway - it just copies nonce field that's already public. Yes that change wasn't necessary in that CL, but it seemed wrong to leave it when I removed the constructors. 2) In the not-so-distant future, Mac will also depend on crypto::RandBytes. > 3) It's not immediately clear to me why you had to move the location of the > code > that uses crypto::RandBytes - why couldn't you have just updated the gyp > files? > That's what I did in the first version of this CL. But GN checks that if a header file is included somewhere then the corresponding target is also in dependency tree. Problem is that it doesn't understand conditional includes. So if dependency is platform-specific then the header must be included in platform-specific file too. See http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... : Running ['/b/build/slave/linux_chromium_gn/build/src/buildtools/linux64/gn', '--root=/b/build/slave/linux_chromium_gn/build/src', 'check', '//out/Release'] ERROR at //ipc/brokerable_attachment.cc:10:11: Include not allowed. #include "crypto/random.h" ^-------------- It is not in any dependency of //ipc:ipc The include file is in the target(s): //crypto:crypto which should somehow be reachable. > https://codereview.chromium.org/1321253011/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/09/14 21:54:17, Sergey Ulanov wrote: > On Mon, Sep 14, 2015 at 1:53 PM, <mailto:erikchen@chromium.org> wrote: > > > On 2015/09/14 17:13:35, Sergey Ulanov wrote: > > > >> Committed patchset #10 (id:260001) manually as > >> 384a29c919f7111c4f50ae9ae56e1599b5faa50a (presubmit successful). > >> > > > > In the future, I'd appreciate it if you cc-ed me on changes that affect > > code > > I've written in the last two weeks. > > > > I'm sorry about it. Initially this CL was intended to be trivial dependency > removal, without any code changes so I sent it to Tom because he is an > owner in ipc. But then I couldn't land that simple version because gn check > was failing (see below), so I had to make other changes, but failed to > include you. > > BTW you can add yourself in the watchlist (see src/WATCHLIST) if you'd like > to be notified about changes in the same directory. > > > > > > 1) Your CL removes SerializeToBuffer(), despite making no mention of it in > > the > > CL description. I'm trying to land a CL right now that depends on the > > existence > > of that function. > > > > 1. That function wasn't used anywhere, i.e. it was dead code. > 2. It didn't look like that function is useful anyway - it just copies > nonce field that's already public. > > Yes that change wasn't necessary in that CL, but it seemed wrong to leave > it when I removed the constructors. > > 2) In the not-so-distant future, Mac will also depend on crypto::RandBytes. > > > 3) It's not immediately clear to me why you had to move the location of the > > code > > that uses crypto::RandBytes - why couldn't you have just updated the gyp > > files? > > > > That's what I did in the first version of this CL. But GN checks that if a > header file is included somewhere then the corresponding target is also in > dependency tree. Problem is that it doesn't understand conditional > includes. So if dependency is platform-specific then the header must be > included in platform-specific file too. See > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > : > > Running > ['/b/build/slave/linux_chromium_gn/build/src/buildtools/linux64/gn', > '--root=/b/build/slave/linux_chromium_gn/build/src', 'check', > '//out/Release'] > ERROR at //ipc/brokerable_attachment.cc:10:11: Include not allowed. > #include "crypto/random.h" > ^-------------- > It is not in any dependency of > //ipc:ipc > The include file is in the target(s): > //crypto:crypto > which should somehow be reachable. > > > > > https://codereview.chromium.org/1321253011/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Note that in this new version of the code, it's possible to construct an uninitialized brokerable attachment, which might appear to have a random nonce (since it's uninitialized) but won't actually. This seems like a serious flaw. It seems like the code got moved around to deal with a limitation of GN? Could we fix GN instead of changing the code in a way that makes it worse?
Message was sent while issue was closed.
On 2015/09/14 22:25:38, erikchen wrote: > On 2015/09/14 21:54:17, Sergey Ulanov wrote: > > On Mon, Sep 14, 2015 at 1:53 PM, <mailto:erikchen@chromium.org> wrote: > > > > > On 2015/09/14 17:13:35, Sergey Ulanov wrote: > > > > > >> Committed patchset #10 (id:260001) manually as > > >> 384a29c919f7111c4f50ae9ae56e1599b5faa50a (presubmit successful). > > >> > > > > > > In the future, I'd appreciate it if you cc-ed me on changes that affect > > > code > > > I've written in the last two weeks. > > > > > > > I'm sorry about it. Initially this CL was intended to be trivial dependency > > removal, without any code changes so I sent it to Tom because he is an > > owner in ipc. But then I couldn't land that simple version because gn check > > was failing (see below), so I had to make other changes, but failed to > > include you. > > > > BTW you can add yourself in the watchlist (see src/WATCHLIST) if you'd like > > to be notified about changes in the same directory. > > > > > > > > > > 1) Your CL removes SerializeToBuffer(), despite making no mention of it in > > > the > > > CL description. I'm trying to land a CL right now that depends on the > > > existence > > > of that function. > > > > > > > 1. That function wasn't used anywhere, i.e. it was dead code. > > 2. It didn't look like that function is useful anyway - it just copies > > nonce field that's already public. > > > > Yes that change wasn't necessary in that CL, but it seemed wrong to leave > > it when I removed the constructors. > > > > 2) In the not-so-distant future, Mac will also depend on crypto::RandBytes. > > > > > 3) It's not immediately clear to me why you had to move the location of the > > > code > > > that uses crypto::RandBytes - why couldn't you have just updated the gyp > > > files? > > > > > > > That's what I did in the first version of this CL. But GN checks that if a > > header file is included somewhere then the corresponding target is also in > > dependency tree. Problem is that it doesn't understand conditional > > includes. So if dependency is platform-specific then the header must be > > included in platform-specific file too. See > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > : > > > > Running > > ['/b/build/slave/linux_chromium_gn/build/src/buildtools/linux64/gn', > > '--root=/b/build/slave/linux_chromium_gn/build/src', 'check', > > '//out/Release'] > > ERROR at //ipc/brokerable_attachment.cc:10:11: Include not allowed. > > #include "crypto/random.h" > > ^-------------- > > It is not in any dependency of > > //ipc:ipc > > The include file is in the target(s): > > //crypto:crypto > > which should somehow be reachable. > > > > > > > > > https://codereview.chromium.org/1321253011/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Note that in this new version of the code, it's possible to construct an > uninitialized brokerable attachment, which might appear to have a random nonce > (since it's uninitialized) but won't actually. This seems like a serious flaw. > > It seems like the code got moved around to deal with a limitation of GN? Could > we fix GN instead of changing the code in a way that makes it worse? I talked with sergeyu. His non-GYP/GN related changes were because GN doesn't handle conditional includes very well (it does dependency checking on includes). """ 9 #if USE_ATTACHMENT_BROKER 10 #include "crypto/random.h" 11 #endif """ Rather than changing the locations where crypto::RandBytes() gets used, which causes a different set of problems, we can make a new class brokerable_attachment_win.cc that uses crypto/rand_bytes.
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/384a29c919f7111c4f50ae9ae56e1599b5faa50a Cr-Commit-Position: refs/heads/master@{#348638} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
