Skip to content

ICE from c_str with null character #16478

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

Closed
jackheizer opened this issue Aug 13, 2014 · 8 comments · Fixed by #17472
Closed

ICE from c_str with null character #16478

jackheizer opened this issue Aug 13, 2014 · 8 comments · Fixed by #17472
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️

Comments

@jackheizer
Copy link
Contributor

Both the export_name and link_section attributes will produce an ICE if '\0' is contained within the string.

Input

#[export_name = "\0"] // link_section will also cause the ICE
static TEST: int = 0;

Output and Bakctrace

error: internal compiler error: unexpected failure
note: the compiler hit an unexpected failure path. this is a bug.
note: we would appreciate a bug report: http://doc.rust-lang.org/complement-bugreport.html
note: run with `RUST_BACKTRACE=1` for a backtrace
task 'rustc' failed at 'assertion failed: *p != 0', /Users/rustbuild/src/rust-buildbot/slave/nightly-mac/build/src/librustrt/c_str.rs:463

stack backtrace:
   1:        0x10fce8b55 - rt::backtrace::imp::write::h62d0bebd377f16937iq
   2:        0x10fcebd13 - failure::on_fail::h5393f2e9d64b17dcCzq
   3:        0x10ffabee5 - unwind::begin_unwind_inner::h7c6fecebc6991c8bS5d
   4:        0x10ffabb1b - unwind::begin_unwind_fmt::h227376fe1e021a36n3d
   5:        0x10ffab972 - rust_begin_unwind
   6:        0x10fffb54c - failure::begin_unwind::h7d8f396ab219c1bbn5j
   7:        0x10cc1b500 - c_str::_&'a [u8].ToCStr::with_c_str::h12322626017237976260
   8:        0x10cbecea7 - middle::trans::base::get_item_val::hec79e19006fbe1f1CTd
   9:        0x10cc79ed7 - middle::trans::consts::trans_const::h7b56de8f9937b6ea5y7
  10:        0x10cbebc22 - middle::trans::base::trans_item::h968ca9f1554c075bbrd
  11:        0x10ccb818c - middle::trans::base::trans_crate::h5c00a13b74652368Ale
  12:        0x10d0babc4 - driver::driver::phase_4_translate_to_llvm::hba8542b14c8284e8wPw
  13:        0x10d0b4257 - driver::driver::compile_input::h7da196e0639e9204csw
  14:        0x10d158437 - driver::run_compiler::hda9adb2662da5f07rWz
  15:        0x10d156bb6 - driver::main_args::closure.136131
  16:        0x10d168b0b - task::TaskBuilder<S>::try_future::closure.137292
  17:        0x10d168a15 - task::TaskBuilder<S>::spawn_internal::closure.137269
  18:        0x10c7d778c - task::spawn_opts::closure.8415
  19:        0x11001122c - rust_try_inner
  20:        0x110011216 - rust_try
  21:        0x10ffa92bb - unwind::try::h5982dbe8fdfe64a5nUd
  22:        0x10ffa905b - task::Task::run::h1c9de674e75b1485v2c
  23:        0x10c7d75ea - task::spawn_opts::closure.8360
  24:        0x10ffaae76 - thread::thread_start::hf75b60e3600e3439Fqd
  25:     0x7fff968ed899 - _pthread_body
  26:     0x7fff968ed72a - _pthread_struct_init

Version

rustc 0.12.0-pre-nightly (e189122e9 2014-08-13 00:41:22 +0000)
@brson
Copy link
Contributor

brson commented Aug 15, 2014

Fascinating find! I'm curious about what lead you to it. Is there a need to put null in these values?

@brson brson added I-wrong and removed I-wrong labels Aug 15, 2014
@jackheizer
Copy link
Contributor Author

I had typed #[export_name = "\01name"] rather than #[export_name = "\x01name"]. It was an accidental discovery, I'm not aware of any need for a null character here.

@brson brson added the E-easy label Aug 16, 2014
@brson
Copy link
Contributor

brson commented Aug 16, 2014

I marked this as easy since fixing this is probably just a matter of going to that exact code and emitting a proper error.

Good starter bug.

@kaseyc
Copy link
Contributor

kaseyc commented Aug 17, 2014

This is actually caused by CStrings failing the task if they have an interior null. source.

Adding an error would fix this particular issue, but it wouldn't fix the underlying issue, so maybe a different course of action would be better.

On a side note, what is the proper way to add an error to the compiler? cx.sess().span_fatal? fail!()? I couldn't find any documentation on it.

@robertg
Copy link

robertg commented Aug 24, 2014

@kaseyc What is the underlying issue? That we should always throw an error if a CString has an interior null character? Or that we should allow CString's to have null characters?

@kaseyc
Copy link
Contributor

kaseyc commented Aug 24, 2014

The underlying issue was that the checked versions of to_c_string and with_c_str crash on interior nulls. Doing an extra interior null check before calling them in this case doesn't help with this problem in other cases, and failing the task seems unnecessary, especially for the checked version. Perhaps the checked version could return an Option, and the unchecked would keep its current behavior.

Edit: Alternately, a to_cstr_opt function could be added and used where necessary.

@caipre
Copy link
Contributor

caipre commented Sep 22, 2014

Is anyone working on this? I'll take a crack at it this week if not.

@kaseyc
Copy link
Contributor

kaseyc commented Sep 22, 2014

I am. I added a with_c_str_opt function (that had to wait on #17120), and am putting the finishing touches on the error messages.

bors added a commit that referenced this issue Sep 24, 2014
Add checks for null bytes in the value strings for the export_name and link_section attributes, reporting an error if any are found, before calling with_c_str on them.

Fixes #16478
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants