Use cstr macro for ffi literal strings #19925

upsuper · 2018-02-01T13:58:17Z

Use cstr!() macro with CStr to ensure that literal strings used with FFI is properly nul-terminated to avoid cases like #19915.

This change is

highfive · 2018-02-01T13:58:21Z

Heads up! This PR modifies the following files:

@bholley: ports/geckolib/lib.rs, ports/geckolib/error_reporter.rs, ports/geckolib/Cargo.toml
@emilio: ports/geckolib/lib.rs, ports/geckolib/error_reporter.rs, ports/geckolib/Cargo.toml

upsuper · 2018-02-01T13:59:23Z

This currently doesn't pass tidy test because it introduces new version of syn and quote, so it depends on #19786. I suppose we need to update those crates eventually, given that cssparser has been using the new versions.

upsuper · 2018-02-01T14:00:07Z

r? @emilio or @nox

cstr is a new crate I wrote, so you may want to review code there as well.

nox · 2018-02-01T14:02:50Z

There was already a terminated crate.

emilio · 2018-02-01T14:05:00Z

I think it's nice to use CStr in this case, seems easier, we don't need guarantees about encoding...

nox · 2018-02-01T14:06:34Z

@emilio Do you mean a macro that returns a CStr, or using CStr directly?

emilio · 2018-02-01T14:22:48Z

I meant the macro, but I don't really care. If you think using another crate is a good call, I'm fine with it :)

upsuper · 2018-02-01T23:10:58Z

Updated the cstr crate to include some more doc and addressed comments from @nox on IRC. The updated cstr crate also removes the dependency to quote crate, so we have one fewer duplicate version package now. (Although that probably doesn't matter as far as it is still blocked on syn.)

emilio · 2018-02-04T13:50:57Z

@bors-servo r+

bors-servo · 2018-02-04T13:50:58Z

📌 Commit d95032c has been approved by emilio

bors-servo · 2018-02-04T13:51:04Z

⌛ Testing commit d95032c with merge 9362d2b...


        Auto merge of #19925 - upsuper:cstr, r=emilio

Use cstr macro for ffi literal strings Use `cstr!()` macro with `CStr` to ensure that literal strings used with FFI is properly nul-terminated to avoid cases like #19915.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19925)

bors-servo · 2018-02-04T14:11:03Z

💔 Test failed - linux-dev

CYBAI · 2018-02-04T15:27:00Z

In ran test-tidy --no-progress --all:

Checking files for tidiness...

./Cargo.lock:1: duplicate versions for package `unicode-xid`
	The following packages depend on version 0.0.4 from 'crates.io':
		syn
		synom
	The following packages depend on version 0.1.0 from 'crates.io':
		proc-macro2
		syn

./Cargo.lock:1: duplicate versions for package `syn`
	The following packages depend on version 0.11.11 from 'crates.io':
		cssparser
		cssparser-macros
		darling_core
		darling_macro
		deny_public_fields
		domobject_derive
		html5ever
		jstraceable_derive
		malloc_size_of_derive
		serde_derive
		serde_derive_internals
		style_derive
		synstructure
	The following packages depend on version 0.12.10 from 'crates.io':
		cstr-macros

Running the dependency licensing lint...

In ran test-stylo:

   Compiling geckoservo v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/ports/geckolib)
error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:397:29
    |
397 |             Action::Drop => cstr!("PEDeclDropped").as_ptr(),
    |                             ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:396:29
    |
396 |             Action::Skip => cstr!("PEDeclSkipped").as_ptr(),
    |                             ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:375:27
    |
375 |                     ) => (cstr!("PEColorNotColor"), Action::Nothing),
    |                           ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:368:18
    |
368 |                 (cstr!("PEUnknownAtRule"), Action::Skip),
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:358:18
    |
358 |                 (cstr!("PEDeclDropped"), Action::Nothing),
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:352:25
    |
352 |                         cstr!("PEDeclDropped")
    |                         ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:349:25
    |
349 |                         cstr!("PEMQNoMinMaxWithoutValue")
    |                         ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:346:25
    |
346 |                         cstr!("PEMQExpectedFeatureValue")
    |                         ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:343:25
    |
343 |                         cstr!("PEMQExpectedFeatureName")
    |                         ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:340:25
    |
340 |                         cstr!("PEGatherMediaNotIdent")
    |                         ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:335:33
    |
335 |                 return (prefix, cstr!("PEBadSelectorRSIgnored"), Action::Nothing);
    |                                 ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:329:34
    |
329 |                             Some(cstr!("PENegationBadArg"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:326:34
    |
326 |                             Some(cstr!("PEClassSelNotIdent"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:323:34
    |
323 |                             Some(cstr!("PEPseudoSelBadName"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:320:34
    |
320 |                             Some(cstr!("PEPseudoClassArgNotIdent"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:317:34
    |
317 |                             Some(cstr!("PEPseudoSelEndOrUserActionPC"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:314:34
    |
314 |                             Some(cstr!("PEPseudoSelUnknown"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:311:34
    |
311 |                             Some(cstr!("PESelectorGroupExtraCombinator"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:308:34
    |
308 |                             Some(cstr!("PESelectorGroupNoSelector"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:305:33
    |
305 |                            Some(cstr!("PEUnknownNamespacePrefix"))
    |                                 ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:302:34
    |
302 |                             Some(cstr!("PETypeSelNotType"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:299:34
    |
299 |                             Some(cstr!("PEAttributeNameExpected"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:296:34
    |
296 |                             Some(cstr!("PEAttributeNameOrNamespaceExpected"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:293:34
    |
293 |                             Some(cstr!("PEAttSelBadValue"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:290:34
    |
290 |                             Some(cstr!("PEAttSelNoBar"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:287:34
    |
287 |                             Some(cstr!("PEAttSelUnexpected"))
    |                                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:281:18
    |
281 |                 (cstr!("PEUnknownAtRule"), Action::Nothing)
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:271:18
    |
271 |                 (cstr!("PEAtNSUnexpected"), Action::Nothing)
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:265:18
    |
265 |                 (cstr!("PEBadSelectorKeyframeRuleIgnored"), Action::Nothing),
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:263:18
    |
263 |                 (cstr!("PEKeyframeBadName"), Action::Nothing),
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:261:18
    |
261 |                 (cstr!("PEUnknownFontDesc"), Action::Skip),
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:259:18
    |
259 |                 (cstr!("PEUnknownProperty"), Action::Drop),
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:255:27
    |
255 |                     _ => (cstr!("PEUnknownProperty"), Action::Drop)
    |                           ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:253:26
    |
253 |                         (cstr!("PEValueParsingError"), Action::Drop)
    |                          ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:250:33
    |
250 |                                 cstr!("PEValueParsingError"), Action::Drop)
    |                                 ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:249:38
    |
249 |                         return (Some(cstr!("PEExpectedNoneOrURLOrFilterFunction")),
    |                                      ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:246:33
    |
246 |                                 cstr!("PEValueParsingError"), Action::Drop)
    |                                 ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:245:38
    |
245 |                         return (Some(cstr!("PEColorNotColor")),
    |                                      ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:238:18
    |
238 |                 (cstr!("PEParseDeclarationDeclExpected"), Action::Skip)
    |                  ^^^^

error: cannot find macro `cstr!` in this scope
   --> ports/geckolib/tests/../../../ports/geckolib/error_reporter.rs:382:26
    |
382 |                         (cstr!("PEDeclDropped"), Action::Nothing)
    |                          ^^^^

error: aborting due to 40 previous errors

error: Could not compile `stylo_tests`.

upsuper · 2018-02-04T22:37:07Z

Fixed the test-stylo issue, but it is still blocked on syn upgrade.

bors-servo · 2018-02-13T17:07:05Z

☔ The latest upstream changes (presumably #20022) made this pull request unmergeable. Please resolve the merge conflicts.


        Use cstr macro for ffi literal strings.

upsuper · 2018-02-14T11:13:17Z

Looks like #20022 bumpped the versions of the dependencies I need here \o/

upsuper · 2018-02-14T11:13:40Z

@bors-servo r=emilio

bors-servo · 2018-02-14T11:13:41Z

📌 Commit 92feec1 has been approved by emilio

bors-servo · 2018-02-14T11:49:40Z

⌛ Testing commit 92feec1 with merge 5af219e...


        Auto merge of #19925 - upsuper:cstr, r=emilio

Use cstr macro for ffi literal strings Use `cstr!()` macro with `CStr` to ensure that literal strings used with FFI is properly nul-terminated to avoid cases like #19915.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19925)

bors-servo · 2018-02-14T12:40:41Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 5af219e to master...

highfive assigned KiChjang Feb 1, 2018

highfive added the S-awaiting-review label Feb 1, 2018

highfive assigned emilio and unassigned KiChjang Feb 1, 2018

upsuper mentioned this pull request Feb 1, 2018

Use a macro to create null-terminated C strings #19916

Closed

upsuper force-pushed the upsuper-forks:cstr branch from 2399214 to d95032c Feb 1, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 4, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 4, 2018

upsuper force-pushed the upsuper-forks:cstr branch from d95032c to 14b0141 Feb 4, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Feb 4, 2018

highfive added the S-needs-rebase label Feb 13, 2018

Use cstr macro for ffi literal strings.

Loading status checks…

92feec1

upsuper force-pushed the upsuper-forks:cstr branch from 14b0141 to 92feec1 Feb 14, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Feb 14, 2018

bors-servo merged commit 92feec1 into servo:master Feb 14, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Feb 14, 2018

upsuper deleted the upsuper-forks:cstr branch Feb 14, 2018

servo / servo

Use cstr macro for ffi literal strings #19925

Use cstr macro for ffi literal strings #19925

upsuper commented Feb 1, 2018 •

edited by SimonSapin

highfive commented Feb 1, 2018

upsuper commented Feb 1, 2018

upsuper commented Feb 1, 2018

nox commented Feb 1, 2018

emilio commented Feb 1, 2018

nox commented Feb 1, 2018 •

edited

emilio commented Feb 1, 2018

upsuper commented Feb 1, 2018

emilio commented Feb 4, 2018

bors-servo commented Feb 4, 2018

bors-servo commented Feb 4, 2018

bors-servo commented Feb 4, 2018

CYBAI commented Feb 4, 2018 •

edited

upsuper commented Feb 4, 2018

bors-servo commented Feb 13, 2018

upsuper commented Feb 14, 2018

upsuper commented Feb 14, 2018

bors-servo commented Feb 14, 2018

bors-servo commented Feb 14, 2018

bors-servo commented Feb 14, 2018

servo / servo

Join GitHub today

Use cstr macro for ffi literal strings #19925

Use cstr macro for ffi literal strings #19925

Conversation

upsuper commented Feb 1, 2018 • edited by SimonSapin

highfive commented Feb 1, 2018

upsuper commented Feb 1, 2018

upsuper commented Feb 1, 2018

nox commented Feb 1, 2018

emilio commented Feb 1, 2018

nox commented Feb 1, 2018 • edited

emilio commented Feb 1, 2018

upsuper commented Feb 1, 2018

emilio commented Feb 4, 2018

bors-servo commented Feb 4, 2018

bors-servo commented Feb 4, 2018

bors-servo commented Feb 4, 2018

CYBAI commented Feb 4, 2018 • edited

upsuper commented Feb 4, 2018

bors-servo commented Feb 13, 2018

upsuper commented Feb 14, 2018

upsuper commented Feb 14, 2018

bors-servo commented Feb 14, 2018

bors-servo commented Feb 14, 2018

bors-servo commented Feb 14, 2018

upsuper commented Feb 1, 2018 •

edited by SimonSapin

nox commented Feb 1, 2018 •

edited

CYBAI commented Feb 4, 2018 •

edited