Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#28077 closed defect (fixed)

remove unsafe block from cstr! macro

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: 0.3.4.1-alpha
Severity: Normal Keywords: rust
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

         .unwrap_or(
            unsafe{
                ::std::ffi::CStr::from_bytes_with_nul_unchecked(b"\0")
            }
        )

Child Tickets

Change History (12)

comment:2 Changed 7 months ago by teor

Status: newneeds_revision

from_bytes_with_nul returns an error if there is more than one nul byte in the string.

In this case, I think we either want to keep the interior nul byte (like the original code) or fail.
Returning an empty string could be a source of subtle bugs.

comment:3 in reply to:  2 ; Changed 7 months ago by cyberpunks

Replying to teor:

keep the interior nul byte (like the original code)

? There's zero behavior change in this patch.

comment:4 in reply to:  3 ; Changed 7 months ago by teor

Replying to cyberpunks:

Replying to teor:

keep the interior nul byte (like the original code)

? There's zero behavior change in this patch.

You're right. I misread the unsafe block.

But I still think we don't want to hide errors by substituting the empty string.

comment:5 in reply to:  4 ; Changed 7 months ago by cyberpunks

Replying to teor:

But I still think we don't want to hide errors by substituting the empty string.

Misuse is extremely unlikely to slip in since this is only used on string literals. But yeah, the ideal solution would be statically asserting at compile-time that the passed literal has no NUL bytes in it, so the only one is the byte being appended.

But defaulting to an empty string(in a case that is basically impossible to get) is the intentional documented behavior of the macro ever since it was first merged in #25185. Improving on that seems like a separate ticket.

Last edited 7 months ago by cyberpunks (previous) (diff)

comment:6 in reply to:  5 Changed 7 months ago by teor

Status: needs_revisionneeds_review

I opened #28079.

I don't have git access right now, so I can't run CI on this branch.
Eventually, someone will open a pull request and review the change.

comment:7 Changed 7 months ago by dgoulet

Milestone: Tor: 0.3.6.x-final
Reviewer: ahf

comment:8 Changed 7 months ago by ahf

The way I read the default() implementation for CStr is that it is:

#[stable(feature = "cstr_default", since = "1.10.0")]
impl<'a> Default for &'a CStr {
    fn default() -> &'a CStr {
        const SLICE: &'static [c_char] = &[0];
        unsafe { CStr::from_ptr(SLICE.as_ptr()) }
    }
}

Which seems to be what we are doing too, which means that to me it seems sensible to use the unwrap_or_default() method.

Opened a PR to see what Travis says: https://github.com/torproject/tor/pull/466

comment:9 Changed 7 months ago by ahf

Status: needs_reviewmerge_ready

The Rust nightly Travis builder fails with:

error: toolchain 'nightly' is not installed

But all the other Travis builders are green, and the code looks good, so I think this is safe to get in.

comment:10 Changed 7 months ago by nickm

merged the pr to master.

comment:11 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

comment:12 Changed 7 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

Note: See TracTickets for help on using tickets.