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

[LLVM 4.0] Don't assume llvm::StringRef is null terminated #38048

Merged
merged 1 commit into from
Dec 1, 2016

Conversation

hanna-kruppe
Copy link
Contributor

@hanna-kruppe hanna-kruppe commented Nov 28, 2016

StringRefs have a length and their contents are not usually null-terminated. The solution is to either copy the string data (in rustc_llvm::diagnostic) or take the size into account (in LLVMRustPrintPasses).

I couldn't trigger a bug caused by this (apparently all the strings returned in practice are actually null-terminated) but this is more correct and more future-proof.

cc #37609

@rust-highfive
Copy link
Collaborator

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@hanna-kruppe hanna-kruppe changed the title Don't assume llvm::StringRef is null terminated [LLVM 4.0] Don't assume llvm::StringRef is null terminated Nov 28, 2016
@hanna-kruppe
Copy link
Contributor Author

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned nrc Nov 28, 2016
@alexcrichton
Copy link
Member

Travis errors seem to indicate that LLVM 3.7 doesn't like this?

StringRefs have a length and their contents are not usually null-terminated.
The solution is to either copy the string data (in rustc_llvm::diagnostic) or take the size into account (in LLVMRustPrintPasses).
I couldn't trigger a bug caused by this (apparently all the strings returned in practice are actually null-terminated) but this is more correct and more future-proof.
@hanna-kruppe
Copy link
Contributor Author

Oops, right! Fixed now, I think.

@alexcrichton
Copy link
Member

r=me, just gonna wait on travis

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 28, 2016

📌 Commit 85dc08e has been approved by alexcrichton

@shepmaster shepmaster mentioned this pull request Dec 1, 2016
23 tasks
@bors
Copy link
Contributor

bors commented Dec 1, 2016

⌛ Testing commit 85dc08e with merge 908dba0...

bors added a commit that referenced this pull request Dec 1, 2016
[LLVM 4.0] Don't assume llvm::StringRef is null terminated

StringRefs have a length and their contents are not usually null-terminated. The solution is to either copy the string data (in `rustc_llvm::diagnostic`) or take the size into account (in LLVMRustPrintPasses).

I couldn't trigger a bug caused by this (apparently all the strings returned in practice are actually null-terminated) but this is more correct and more future-proof.

cc #37609
@bors bors merged commit 85dc08e into rust-lang:master Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants