Arthur Sonzogni (Gerrit)

unread,

9:43 AM (2 hours ago) 9:43 AM

to Egor Pasko, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, blink-...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kinuko...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mac-r...@chromium.org, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org

Attention needed from Egor Pasko

Arthur Sonzogni added 2 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Arthur Sonzogni . resolved

Hi Egor,

File base/android/linker/DEPS

Line 9, Patchset 4 (Latest):

Arthur Sonzogni . unresolved

Adding this specific dependency should be fine. It contains only macros definition. No C++ declaration / definition, so this shouldn't link in anything and/or duplicate symbols from //base.

WDYT @pa...@chromium.org?

Alternatively, we could duplicate the macro into a //base/android/link/compiler_specific.h
file like in the very first few patchsets.

Open in Gerrit

Related details

Attention is currently required from:

Egor Pasko

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Egor Pasko (Gerrit)

unread,

10:05 AM (2 hours ago) 10:05 AM

to Arthur Sonzogni, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, blink-...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kinuko...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mac-r...@chromium.org, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org

Attention needed from Arthur Sonzogni

Egor Pasko voted and added 3 comments

Votes added by Egor Pasko

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Egor Pasko . resolved

lgtm

File base/android/linker/DEPS

Line 9, Patchset 4 (Latest):

Arthur Sonzogni . resolved

Adding this specific dependency should be fine. It contains only macros definition. No C++ declaration / definition, so this shouldn't link in anything and/or duplicate symbols from //base.
WDYT @pa...@chromium.org?

Alternatively, we could duplicate the macro into a //base/android/link/compiler_specific.h
file like in the very first few patchsets.

Egor Pasko

The macros in //base/compiler_specific.h look innocent to me for the purpose, so lgtm.

PS: Even if later the macros start calling functions from //base, it will cause unknown symbol link errors (because we do not provide libbase to it). I think these include rules are serving only as contract declaration, not the last line of defense.

File base/android/linker/linker_jni.cc

Line 174, Patchset 4 (Latest): UNSAFE_TODO(memcpy(buf, buf + kReadSize, kMaxLineLength));

Egor Pasko . unresolved

Q: how are we going to eliminate the UNSAFE_TODO for a library that cannot depend on //base and cannot use std::span? What is the plan?

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

10:29 AM (1 hour ago) 10:29 AM

to Egor Pasko, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, blink-...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kinuko...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mac-r...@chromium.org, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org

Arthur Sonzogni voted and added 2 comments

Votes added by Arthur Sonzogni

Commit-Queue

+2

2 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Arthur Sonzogni . resolved

Thanks!

File base/android/linker/linker_jni.cc

Line 174, Patchset 4 (Latest): UNSAFE_TODO(memcpy(buf, buf + kReadSize, kMaxLineLength));

Egor Pasko . resolved

Q: how are we going to eliminate the UNSAFE_TODO for a library that cannot depend on //base and cannot use std::span? What is the plan?

Arthur Sonzogni

A plan? ;-)

Without joking, For `partition_alloc`, we imported our own `base::span`. `base/android/linker/` is too small to justify it.

We still can:

Use `std::array` and "modern" C++ API considered safe.
Allow `std::span` in this directory and use it.
Use UNSAFE_BUFFERS(...) with justifications.

I can give it a try in a follow-up and see what could be achieved quickly.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

10:35 AM (1 hour ago) 10:35 AM

to Arthur Sonzogni, Egor Pasko, Akihiro Ota, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, blink-...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kinuko...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mac-r...@chromium.org, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[unsafe-buffers] Remove allow_unsafe_buffers from linker_jni.cc

This CL removes '#pragma allow_unsafe_buffers' from linker_jni.cc.
It defines a local LINKER_UNSAFE_TODO macro because this file cannot depend on
base/, and wraps unsafe buffer usages.
This is a mechanical change transitioning to fine-grained macros.

Bug: 351564777, 501112808

Doc: go/complete_unsafe_todo_migration

Change-Id: I704a137930e662bf4a019b81ecfaa9ad679daa74

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8032730

Commit-Queue: Arthur Sonzogni <arthurs...@chromium.org>

Reviewed-by: Egor Pasko <pa...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1657181}

Files:

M base/android/linker/BUILD.gn
M base/android/linker/DEPS
M base/android/linker/linker_jni.cc

Change size: S

Delta: 3 files changed, 23 insertions(+), 23 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Egor Pasko

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Egor Pasko (Gerrit)

unread,

10:55 AM (1 hour ago) 10:55 AM

to Chromium LUCI CQ, Arthur Sonzogni, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Sadrul Chowdhury, Zhe Su, abigailbk...@google.com, agriev...@chromium.org, arc-review...@google.com, bartek...@chromium.org, blink-...@chromium.org, caitkp...@chromium.org, dcheng+c...@chromium.org, derinel+wat...@google.com, dtseng...@chromium.org, francisjp...@google.com, gcasto+w...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, ios-r...@chromium.org, jessemcke...@google.com, josiah...@chromium.org, jshin...@chromium.org, keithle...@chromium.org, kinuko...@chromium.org, kyungjunle...@google.com, lizeb...@chromium.org, lucasrada...@google.com, mac-r...@chromium.org, mattreyno...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, nona+...@chromium.org, nyquis...@chromium.org, ortuno...@chromium.org, pasko...@chromium.org, pmonett...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, shuche...@chromium.org, sky+...@chromium.org, titoua...@chromium.org, torne...@chromium.org, tranbaod...@chromium.org, vasilii+watchlis...@chromium.org, webauthn...@chromium.org, wfh+...@chromium.org, yfriedm...@chromium.org, yhanad...@chromium.org, yhanada+...@chromium.org, yuzo+...@chromium.org

Egor Pasko added 1 comment

File base/android/linker/linker_jni.cc

Line 174, Patchset 4: UNSAFE_TODO(memcpy(buf, buf + kReadSize, kMaxLineLength));

Egor Pasko . resolved

Q: how are we going to eliminate the UNSAFE_TODO for a library that cannot depend on //base and cannot use std::span? What is the plan?

Arthur Sonzogni

A plan? ;-)
Without joking, For `partition_alloc`, we imported our own `base::span`. `base/android/linker/` is too small to justify it.
We still can:
Use `std::array` and "modern" C++ API considered safe.
Allow `std::span` in this directory and use it.
Use UNSAFE_BUFFERS(...) with justifications.

I can give it a try in a follow-up and see what could be achieved quickly.

Egor Pasko

Oh, that's very kind of you. Thanks!!

This code parses strings and walks ELF file format. I'm not sure it is easy to make these things provably memory-safe without extra overhead. We somewhat rely on data being well-formed (it was emitted by kernel or validated by the dynamic linker).

A plan? ;-)

Well, because "TODO" is in the name, I ought to naively assume that there is a plan ;))

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[unsafe-buffers] Remove allow_unsafe_buffers from linker_jni.cc [chromium/src : main]

Arthur Sonzogni (Gerrit)

Arthur Sonzogni added 2 comments

Related details

Egor Pasko (Gerrit)

Egor Pasko voted and added 3 comments

Votes added by Egor Pasko

3 comments

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni voted and added 2 comments

Votes added by Arthur Sonzogni

2 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information

Egor Pasko (Gerrit)

Egor Pasko added 1 comment

Related details