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

Unsoundness from premature drop of temporary in match scrutinee #10683

Closed
kmcallister opened this issue Nov 27, 2013 · 10 comments
Closed

Unsoundness from premature drop of temporary in match scrutinee #10683

kmcallister opened this issue Nov 27, 2013 · 10 comments
Labels
A-codegen Area: Code generation E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.

Comments

@kmcallister
Copy link
Contributor

We have some code in Servo of the form:

match name.to_ascii_lower().as_slice() {
    "margin-top"   =>  // stuff
    "margin-right" =>  // stuff
    // dozens more of these
}

The match will sometimes (nondeterministically) fail even though name matches one of the listed string literals. The bug apparently disappears if I change the code to

let name_lower = name.to_ascii_lower();
match name_lower.as_slice() {

So I believe the ~str returned by to_ascii_lower() is getting freed before the match is done. (I couldn't reproduce the bug inside Valgrind, however.)

You can see the actual Servo code here. It's a Mako template which outputs Rust code. If you build Servo you can see the generated code at src/components/style/properties.rs.

We're using

rustc 0.9-pre (67d7be0 2013-10-29 12:02:59 -0700)

so if you believe this bug is already fixed in master, let us know and we can upgrade.

@kmcallister
Copy link
Contributor Author

Note that the Servo bug seems to go away when optimization is enabled.

@kmcallister
Copy link
Contributor Author

Here's some relevant (?) disassembly with the local variable:

   49d8c:       e8 0f 68 fe ff          callq  305a0 <_ZN5ascii26StrAsciiExt$__extensions__14to_ascii_lower19h2742b85a3a278c3GaH4v0.1E>
   49d91:       48 89 45 c0             mov    %rax,-0x40(%rbp)
   49d95:       48 8d 7d b0             lea    -0x50(%rbp),%rdi
   49d99:       48 8d 75 c0             lea    -0x40(%rbp),%rsi
   49d9d:       e8 ee 76 fe ff          callq  31490 <_ZN3str18Str$__extensions__8as_slice20h13b4445a0db8813U4am4v0.1E>
   49da2:       eb 00                   jmp    49da4 <_ZN10properties19PropertyDeclaration5parse19h586366822340e1rhad4v0.1E+0x84>
   49da4:       48 8d 05 89 a5 09 00    lea    0x9a589(%rip),%rax        # e4334 <str8207>
   49dab:       48 89 45 90             mov    %rax,-0x70(%rbp)
   49daf:       48 c7 45 98 0a 00 00    movq   $0xa,-0x68(%rbp)
   49db6:       00 
   49db7:       48 8d 75 b0             lea    -0x50(%rbp),%rsi
   49dbb:       48 8d 55 90             lea    -0x70(%rbp),%rdx
   49dbf:       e8 4c 77 fe ff          callq  31510 <_ZN3str8eq_slice19h6fa631b3d59b91e0aZ4v0.1E>
   49dc4:       88 85 07 77 ff ff       mov    %al,-0x88f9(%rbp)
   49dca:       e9 27 10 00 00          jmpq   4adf6 <_ZN10properties19PropertyDeclaration5parse19h586366822340e1rhad4v0.1E+0x10d6>

and without:

   49d9c:       e8 0f 68 fe ff          callq  305b0 <_ZN5ascii26StrAsciiExt$__extensions__14to_ascii_lower19h2742b85a3a278c3GaF4v0.1E>
   49da1:       48 89 45 b0             mov    %rax,-0x50(%rbp)
   49da5:       48 89 45 a8             mov    %rax,-0x58(%rbp)
   49da9:       48 8d 7d b8             lea    -0x48(%rbp),%rdi
   49dad:       48 8d 75 a8             lea    -0x58(%rbp),%rsi
   49db1:       e8 ea 76 fe ff          callq  314a0 <_ZN3str18Str$__extensions__8as_slice20h13b4445a0db8813U4ak4v0.1E>
   49db6:       eb 00                   jmp    49db8 <_ZN10properties19PropertyDeclaration5parse19h586366822340e1rhab4v0.1E+0x88>
   49db8:       48 bf 00 00 00 00 00    movabs $0x0,%rdi
   49dbf:       00 00 00 
   49dc2:       48 8d 75 a8             lea    -0x58(%rbp),%rsi
   49dc6:       e8 15 1e fd ff          callq  1bbe0 <_ZN8_$UP$str9glue_drop19h62ec8c421db32e66a5E>
   49dcb:       48 8d 75 b8             lea    -0x48(%rbp),%rsi
   49dcf:       48 8d 55 88             lea    -0x78(%rbp),%rdx
   49dd3:       48 8d 05 da a1 09 00    lea    0x9a1da(%rip),%rax        # e3fb4 <str8205>
   49dda:       48 89 45 88             mov    %rax,-0x78(%rbp)
   49dde:       48 c7 45 90 0a 00 00    movq   $0xa,-0x70(%rbp)
   49de5:       00 
   49de6:       e8 35 77 fe ff          callq  31520 <_ZN3str8eq_slice19h6fa631b3d59b91e0aU4v0.1E>
   49deb:       3c 00                   cmp    $0x0,%al
   49ded:       0f 85 0e 15 00 00       jne    4b301 <_ZN10properties19PropertyDeclaration5parse19h586366822340e1rhab4v0.1E+0x15d1>
   49df3:       e9 0e 15 00 00          jmpq   4b306 <_ZN10properties19PropertyDeclaration5parse19h586366822340e1rhab4v0.1E+0x15d6>

jmp 49db8 is strange, because it jumps to the very next instruction (but this is a no-optimize build). Anyway it seems to be calling ~str::glue_drop on the result of to_ascii_lower.

I can provide the .ll files on request but they're 16 MB each.

@huonw
Copy link
Member

huonw commented Dec 3, 2013

use std::ascii::StrAsciiExt;

static NAME: &'static str = "hello world";

#[cfg(not(with_local))]
fn main() {
    match NAME.to_ascii_lower().as_slice() {
        "foo" => {}
        _ => {}
    }
}
#[cfg(with_local)]
fn main() {
    let name = NAME.to_ascii_lower();
    match name.as_slice() {
        "foo" => {}
        _ => {}
    }
}

seems to display the same problem, that is, when compiled without --cfg with_local glue_drop for ~str is called before eq_slice, but with --cfg with_local, eq_slice is called first.

IR for --cfg with_local:

; Function Attrs: uwtable
define internal void @_ZN4main18hee8776918765f67an4v0.0E({ i64, %tydesc*, i8*, i8*, i8 }*) unnamed_addr #4 {
"function top level":
  %name = alloca { i64, i64, [0 x i8] }*
  %1 = alloca %str_slice
  %2 = alloca { i8*, i32 }
  %3 = alloca %str_slice
  %4 = call { i64, i64, [0 x i8] }* @"_ZN5ascii26StrAsciiExt$__extensions__14to_ascii_lower20h4985d7ef49cc8d0zCaY4v0.0E"({ i64, %tydesc*, i8*, i8*, i8 }* bitcast ({ i8*, i64 }* @_ZN4NAME18h2f2e2b20757b714ah4v0.0E to { i64, %tydesc*, i8*, i8*, i8 }*))
  store { i64, i64, [0 x i8] }* %4, { i64, i64, [0 x i8] }** %name
  %5 = bitcast { i64, i64, [0 x i8] }** %name to { i64, %tydesc*, i8*, i8*, i8 }*
  invoke void @"_ZN3str18Str$__extensions__8as_slice21h994e7dba4c3712c06vaL4v0.0E"(%str_slice* sret %1, { i64, %tydesc*, i8*, i8*, i8 }* %5)
          to label %"normal return" unwind label %unwind

"normal return":                                  ; preds = %"function top level"
  %6 = getelementptr inbounds %str_slice* %3, i32 0, i32 0
  store i8* getelementptr inbounds ([4 x i8]* @str1797, i32 0, i32 0), i8** %6
  %7 = getelementptr inbounds %str_slice* %3, i32 0, i32 1
  store i64 3, i64* %7
  %8 = invoke i8 @_ZN3str8eq_slice19h85916ee1bf3841d9a84v0.0E({ i64, %tydesc*, i8*, i8*, i8 }* undef, %str_slice* %1, %str_slice* %3)
          to label %"normal return2" unwind label %unwind

unwind:                                           ; preds = %"normal return", %"function top level"
  %9 = landingpad { i8*, i32 } personality i32 ()* @upcall_rust_personality
          cleanup
  call void @upcall_reset_stack_limit()
  store { i8*, i32 } %9, { i8*, i32 }* %2
  br label %cleanup

cleanup:                                          ; preds = %unwind
  call void @"_ZN8_$UP$str9glue_drop19h7187567e28f1bbb6aXE"({}* null, { i64, i64, [0 x i8] }** %name)
  %10 = load { i8*, i32 }* %2
  resume { i8*, i32 } %10

case_body:                                        ; preds = %match_case
  br label %join

case_body1:                                       ; preds = %match_else
  br label %join

match_else:                                       ; preds = %compare_next
  br label %case_body1

match_case:                                       ; preds = %"normal return2"
  br label %case_body

"normal return2":                                 ; preds = %"normal return"
  %11 = icmp ne i8 %8, 0
  br i1 %11, label %match_case, label %compare_next

compare_next:                                     ; preds = %"normal return2"
  br label %match_else

join:                                             ; preds = %case_body1, %case_body
  call void @"_ZN8_$UP$str9glue_drop19h7187567e28f1bbb6aXE"({}* null, { i64, i64, [0 x i8] }** %name)
  ret void
}

without --cfg with_local:

; Function Attrs: uwtable
define internal void @_ZN4main18hee8776918765f67an4v0.0E({ i64, %tydesc*, i8*, i8*, i8 }*) unnamed_addr #4 {
"function top level":
  %1 = alloca %str_slice
  %2 = alloca { i64, i64, [0 x i8] }*
  %3 = alloca { i64, i64, [0 x i8] }*
  %4 = alloca { i8*, i32 }
  %5 = alloca %str_slice
  %6 = call { i64, i64, [0 x i8] }* @"_ZN5ascii26StrAsciiExt$__extensions__14to_ascii_lower20h4985d7ef49cc8d0zCa04v0.0E"({ i64, %tydesc*, i8*, i8*, i8 }* bitcast ({ i8*, i64 }* @_ZN4NAME18h2f2e2b20757b714ah4v0.0E to { i64, %tydesc*, i8*, i8*, i8 }*))
  store { i64, i64, [0 x i8] }* %6, { i64, i64, [0 x i8] }** %2
  %7 = load { i64, i64, [0 x i8] }** %2
  store { i64, i64, [0 x i8] }* %7, { i64, i64, [0 x i8] }** %3
  %8 = bitcast { i64, i64, [0 x i8] }** %3 to { i64, %tydesc*, i8*, i8*, i8 }*
  invoke void @"_ZN3str18Str$__extensions__8as_slice21h994e7dba4c3712c06vaY4v0.0E"(%str_slice* sret %1, { i64, %tydesc*, i8*, i8*, i8 }* %8)
          to label %"normal return" unwind label %unwind

"normal return":                                  ; preds = %"function top level"
  call void @"_ZN8_$UP$str9glue_drop19h7187567e28f1bbb6aYE"({}* null, { i64, i64, [0 x i8] }** %3)
  %9 = getelementptr inbounds %str_slice* %5, i32 0, i32 0
  store i8* getelementptr inbounds ([4 x i8]* @str1796, i32 0, i32 0), i8** %9
  %10 = getelementptr inbounds %str_slice* %5, i32 0, i32 1
  store i64 3, i64* %10
  %11 = call i8 @_ZN3str8eq_slice19h85916ee1bf3841d9a34v0.0E({ i64, %tydesc*, i8*, i8*, i8 }* undef, %str_slice* %1, %str_slice* %5)
  %12 = icmp ne i8 %11, 0
  br i1 %12, label %match_case, label %compare_next

unwind:                                           ; preds = %"function top level"
  %13 = landingpad { i8*, i32 } personality i32 ()* @upcall_rust_personality
          cleanup
  call void @upcall_reset_stack_limit()
  store { i8*, i32 } %13, { i8*, i32 }* %4
  br label %cleanup

cleanup:                                          ; preds = %unwind
  call void @"_ZN8_$UP$str9glue_drop19h7187567e28f1bbb6aYE"({}* null, { i64, i64, [0 x i8] }** %3)
  %14 = load { i8*, i32 }* %4
  resume { i8*, i32 } %14

case_body:                                        ; preds = %match_case
  br label %join

case_body1:                                       ; preds = %match_else
  br label %join

match_else:                                       ; preds = %compare_next
  br label %case_body1

match_case:                                       ; preds = %"normal return"
  br label %case_body

compare_next:                                     ; preds = %"normal return"
  br label %match_else

join:                                             ; preds = %case_body1, %case_body
  ret void
}

The key point being the positions of the call void @"_ZN8_$UP$str9glue_drop19h7187567e28f1bbb6aYE" call in each.

kmcallister added a commit to kmcallister/servo that referenced this issue Dec 3, 2013
The bug is rust-lang/rust#10683 and there's no fix yet, plus it
would take us a while to upgrade Rust anyway.

Fixes servo#1258.
bors-servo pushed a commit to servo/servo that referenced this issue Dec 3, 2013
The bug is rust-lang/rust#10683 and there's no fix yet, plus it would take us a while to upgrade Rust anyway.

Fixes #1258.
@nikomatsakis
Copy link
Contributor

cc me

@metajack
Copy link
Contributor

metajack commented Jan 6, 2014

This is not yet fixed, or at least not all fixed.

For example, Rust master-ish with rust-encoding fails but only on OS X. Assigning the result of to_ascii_lower() to a temp then matching on temp.as_slice() fixes the issue.

See here: https://github.com/mozilla-servo/rust-encoding/blob/master/src/label.rs#L14

@metajack
Copy link
Contributor

metajack commented Jan 7, 2014

pinging @hyunjunekim

@hyunjunekim
Copy link

pong @metajack about this issue, What to have the problem in pull request (servo/servo#1388)?

@metajack
Copy link
Contributor

metajack commented Jan 9, 2014

rust-encoding still crashes with this error even with your changes from #1388. Maybe more changes are needed?

@nikomatsakis
Copy link
Contributor

Can someone retest this easily with the latest rust master? (I don't have ready access to a rustc right at this moment) Quite possibly fixed by #3511

@pnkfelix
Copy link
Member

huonw's similar example seems to work now. But it would be good to check against the original bug. (And it might also be good to put huon's example as a regression test.)

therealglazou pushed a commit to therealglazou/servo that referenced this issue Feb 20, 2014
The bug is rust-lang/rust#10683 and there's no fix yet, plus it
would take us a while to upgrade Rust anyway.

Fixes servo#1258.
therealglazou pushed a commit to therealglazou/servo that referenced this issue Feb 20, 2014
bors added a commit that referenced this issue Apr 28, 2014
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Sep 30, 2019
…S parse failure (from kmcallister:properties); r=jdm

The bug is rust-lang/rust#10683 and there's no fix yet, plus it would take us a while to upgrade Rust anyway.

Fixes #1258.

Source-Repo: https://github.com/servo/servo
Source-Revision: 148d1720e539c36ed44efdc7ebf3e45f25388f13

UltraBlame original commit: 28bc03c8fa39fea27af15f247cb7cc3c3f6fe31d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Sep 30, 2019
…unjunekim:cssissues); r=kmcallister

Fixed this issue ( rust-lang/rust#10683 )

Source-Repo: https://github.com/servo/servo
Source-Revision: c5d81f13c167ba46836b78359389e8d1a3f817d6

UltraBlame original commit: 19615f9ab014ee097b16427c1c85db9c1a353b6d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Sep 30, 2019
…S parse failure (from kmcallister:properties); r=jdm

The bug is rust-lang/rust#10683 and there's no fix yet, plus it would take us a while to upgrade Rust anyway.

Fixes #1258.

Source-Repo: https://github.com/servo/servo
Source-Revision: 148d1720e539c36ed44efdc7ebf3e45f25388f13

UltraBlame original commit: 28bc03c8fa39fea27af15f247cb7cc3c3f6fe31d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Sep 30, 2019
…unjunekim:cssissues); r=kmcallister

Fixed this issue ( rust-lang/rust#10683 )

Source-Repo: https://github.com/servo/servo
Source-Revision: c5d81f13c167ba46836b78359389e8d1a3f817d6

UltraBlame original commit: 19615f9ab014ee097b16427c1c85db9c1a353b6d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…S parse failure (from kmcallister:properties); r=jdm

The bug is rust-lang/rust#10683 and there's no fix yet, plus it would take us a while to upgrade Rust anyway.

Fixes #1258.

Source-Repo: https://github.com/servo/servo
Source-Revision: 148d1720e539c36ed44efdc7ebf3e45f25388f13

UltraBlame original commit: 28bc03c8fa39fea27af15f247cb7cc3c3f6fe31d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…unjunekim:cssissues); r=kmcallister

Fixed this issue ( rust-lang/rust#10683 )

Source-Repo: https://github.com/servo/servo
Source-Revision: c5d81f13c167ba46836b78359389e8d1a3f817d6

UltraBlame original commit: 19615f9ab014ee097b16427c1c85db9c1a353b6d
flip1995 pushed a commit to flip1995/rust that referenced this issue May 5, 2023
Fix false positive in `allow_attributes`

This would emit a warning if used in a proc-macro with the feature `lint_reasons` enabled. This is now fixed.

changelog: [`allow_attributes`]: Don't lint if in external macro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants