Opened 12 months ago

Closed 11 months ago

Last modified 11 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 12 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 12 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 12 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 12 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 12 months ago by cyberpunks (previous) (diff)

comment:6 in reply to:  5 Changed 12 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 12 months ago by dgoulet

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

comment:8 Changed 12 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 12 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 11 months ago by nickm

merged the pr to master.

comment:11 Changed 11 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

comment:12 Changed 11 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.