Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not leak the CAPI strings #23998

Merged
merged 1 commit into from Aug 22, 2019
Merged

Do not leak the CAPI strings #23998

merged 1 commit into from Aug 22, 2019

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Aug 19, 2019

Depends on #23983

I haven't remove the mem::forget here:

pub extern "C" fn servo_version() -> *const c_char {

What would be the right approach in that specific case?


This change is Reviewable

@highfive
Copy link

highfive commented Aug 19, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@paulrouget paulrouget force-pushed the paulrouget:rmForget branch 2 times, most recently from 1137aa1 to b41e097 Aug 20, 2019
@jdm
Copy link
Member

jdm commented Aug 21, 2019

Any chance servo_version() can return a static string? The changes in this PR look fine to me.

@paulrouget paulrouget force-pushed the paulrouget:rmForget branch from b41e097 to 700051b Aug 21, 2019
@paulrouget paulrouget changed the title [WIP] do not leak the CAPI strings Do not leak the CAPI strings Aug 21, 2019
@paulrouget
Copy link
Contributor Author

paulrouget commented Aug 21, 2019

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

📌 Commit 700051b has been approved by jdm

@highfive highfive assigned jdm and unassigned SimonSapin Aug 21, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

Testing commit 700051b with merge 293103f...

bors-servo added a commit that referenced this pull request Aug 21, 2019
Do not leak the CAPI strings

Depends on #23983

I haven't remove the mem::forget here: https://github.com/servo/servo/blob/6528c0d9b7b36fbdffc29bab99e0066b83a7f970/ports/libsimpleservo/capi/src/lib.rs#L228

What would be the right approach in that specific case?

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23998)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

💔 Test failed - status-taskcluster

@CYBAI
Copy link
Collaborator

CYBAI commented Aug 21, 2019

Failed at ./mach build --dev --libsimpleservo

   Compiling libservo v0.0.1 (/repo/components/servo)
error: unused import: `std::mem`
  --> ports/libsimpleservo/capi/src/lib.rs:20:5
   |
20 | use std::mem;
   |     ^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

error: aborting due to previous error

error: Could not compile `simpleservo_capi`.
@paulrouget paulrouget force-pushed the paulrouget:rmForget branch from 700051b to fe35883 Aug 21, 2019
@paulrouget
Copy link
Contributor Author

paulrouget commented Aug 21, 2019

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

📌 Commit fe35883 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2019

Testing commit fe35883 with merge 52b63b7...

bors-servo added a commit that referenced this pull request Aug 22, 2019
Do not leak the CAPI strings

Depends on #23983

I haven't remove the mem::forget here: https://github.com/servo/servo/blob/6528c0d9b7b36fbdffc29bab99e0066b83a7f970/ports/libsimpleservo/capi/src/lib.rs#L228

What would be the right approach in that specific case?

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23998)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 52b63b7 to master...

@bors-servo bors-servo merged commit fe35883 into servo:master Aug 22, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.