1

I am writing a recursive type ListNode in Rust. I have to use Box in the struct and I am trying to write a loop to add next ListNode. However, I would like to try using a pointer except recursive method.

#[derive(Debug)]
struct ListNode {
    val: i32,
    next: Option<Box<ListNode>>,
}

impl ListNode {
    fn new(i: i32) -> Self {
        ListNode { val: i, next: None }
    }

    fn add_l(&mut self, l: &Vec<i32>) {
        let mut p: *mut ListNode = self as *mut ListNode;
        for i in l {
            unsafe {
                (*p).next = Some(Box::new(ListNode::new(*i)));
                let temp_b = Box::from_raw(p);
                p = Box::into_raw(temp_b.next.wrap());
            };
        }
    }
}

fn main() {
    let mut a = ListNode::new(1);
    a.add_l(&vec![2, 3, 4, 5]);
    println!("{:?}", a);
}

I found that a is changed to the last NodeList with the val of 5:

ListNode { val: 5, next: None }
  1. Is there any way I can copy a pointer so I can keep a stable?
  2. If there is no way I can copy pointer, how can I implement this?
Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
ccQpein
  • 705
  • 7
  • 20
  • 4
    *I would like to try using a pointer* — one of the biggest reasons to use Rust is to **avoid** using pointers when possible. – Shepmaster Jun 20 '18 at 22:11
  • 2
    The first time you hit that `Box::from_raw`, the pointer you pass it is not derived from a `Box`. I'm pretty confused by what you're trying to do here and I don't understand what it has to do with copying a pointer. [Raw pointers are trivially copyable](http://play.rust-lang.org/?gist=64eddadd117c9fd9c9f8b0167b154cb2&version=stable&mode=debug). – trent Jun 20 '18 at 22:24
  • If I do not create `temp_b` to keep pointer, `p = Box::into_raw((*p).next.unwrap());` will give me `cannot move out of dereference of raw pointer` error when I am compiling – ccQpein Jun 20 '18 at 22:29
  • Obligatory link to [*Learning Rust With Entirely Too Many Linked Lists*](http://cglab.ca/~abeinges/blah/too-many-lists/book/) :) Chapter 6 onwards demonstrates how to write a data structure using `unsafe`. Although I'll second what Shepmaster and Doug said - you're probably better off sticking with `Rc` (which is what the earlier chapters show you how to do) unless you have a good reason not to do so. – Joe Clay Jun 21 '18 at 10:44

2 Answers2

7

First things first: using unsafe here is completely unnecessary and I would say actively malicious if I saw it in any real code. Don't use unsafe for "fun".

Here's a completely safe implementation of the function which walks backwards to construct the new tail of the list to add:

fn add_l(&mut self, l: &[i32]) {
    let mut tail = None;

    for &val in l.iter().rev() {
        let next = tail.take();
        tail = Some(Box::new(ListNode { val, next }));
    }

    self.next = tail;
}

And one that goes forwards, but requires an unwrap:

fn add_l(&mut self, l: &[i32]) {
    let mut head = self;

    for &val in l {
        head.next = Some(Box::new(ListNode::new(val)));
        head = { head }.next.as_mut().unwrap();
    }
}

If you had to do it in the forwards direction and had to avoid the unwrap, then maybe you could use unsafe. Every single unsafe block should contain a wealth of comments explaining how the code is safe and doesn't break the guarantees that you need to uphold.

fn add_l(&mut self, l: &[i32]) {
    let mut head = self;

    for &val in l {
        unsafe {
            // Boxing a value gives it a stable address.
            let mut node = Box::new(ListNode::new(val));

            // So long as this raw pointer doesn't escape this block, 
            // we don't need to worry about its lifetime as it should 
            // outlive what we need.
            let node_raw = &mut node as &mut ListNode as *mut ListNode;

            head.next = Some(node);

            // Now that we've moved the `Box` into its final place,
            // we throw away the reference to head to avoid mutable 
            // aliasing
            head = &mut *node_raw;
        }
    }
}

See also:

Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
3

You have a number of issues here, none of which relate to copying a pointer.

I see what you're trying to do, but you're seeing 'undefined behavior' in action, not a failure to copy the pointer value.

First of all, this:

temp_b.next.unwrap()

Unwrap does not leave the object behind; it consumes it. Every iteration through, you will be setting the value of next to nothing as you call unwrap.

Secondly, on the first iteration through your loop, you convert the original pointer into a box:

let mut p: *mut ListNode = self as *mut ListNode;
// ... p is not reassigned before calling the next statement
Box::from_raw(p);

As a result, you're dropping (free'ing) the root object a when you consume temp_b.

This won't immediately trigger a crash, but it means you've now effectively corrupted the stack. Everything past this point is undefined behavior.

Look at the output when you trace your actual pointer values:

#[derive(Debug)]
struct ListNode {
    val: String,
    next: Option<Box<ListNode>>,
}

impl ListNode {
    fn new(i: &str) -> Self {
        ListNode { val: format!("{:?}", i), next: None }
    }

    fn add_l(&mut self, l: &Vec<&str>) {
        let mut p: *mut ListNode = self as *mut ListNode;
        println!("self -> {:?}", self as *mut ListNode);
        for i in l {
            unsafe {
                (*p).next = Some(Box::new(ListNode::new(*i)));
                let temp_b = Box::from_raw(p);
                println!("{:?} -> {:?}", p, temp_b);
                p = Box::into_raw(temp_b.next.unwrap());
                println!("next p -> {:?}", p);
            };

        }
        println!("self -> {:?}", self as *mut ListNode);
    }
}

fn main() {
    let mut a = ListNode::new("1");
    a.add_l(&vec!["2", "3", "4", "5"]);
    println!("self -> {:?}", &mut a as *mut ListNode);
    println!("{:?}", a);
}

...

self -> 0x7ffdc10a90f0
0x7ffdc10a90f0 -> ListNode { val: "\"1\"", next: Some(ListNode { val: "\"2\"", next: None }) }
next p -> 0x7fdde801f060
0x7fdde801f060 -> ListNode { val: "\"2\"", next: Some(ListNode { val: "\"3\"", next: None }) }
next p -> 0x7ffdc10a90f0
0x7ffdc10a90f0 -> ListNode { val: "\"3\"", next: Some(ListNode { val: "\"4\"", next: None }) }
next p -> 0x7fdde801f060
0x7fdde801f060 -> ListNode { val: "\"4\"", next: Some(ListNode { val: "\"5\"", next: None }) }
next p -> 0x7ffdc10a90f0 <---- Whhhaaaat! You've been reallocated!
self -> 0x7ffdc10a90f0
self -> 0x7ffdc10a90f0
ListNode { val: "\"5\"", next: None }

So... this is why using unsafe is unsafe.

You can't do what you want to do without using raw pointers all the way; I recommend you look at Rc for what you're trying to do.

Doug
  • 32,844
  • 38
  • 166
  • 222