Memory unsoundness in port_info (and maybe others)

[RReverser]

In the process of writing tests, I've noticed that the internal pointer of camera.port_info() seems to be tied to the Camera's own lifetime, and is freed when the camera is.

As a result, it's very easy to run into memory corruption issues in safe code. Example:

let camera = Context::new()?.autodetect_camera()?;
let port_info = camera.port_info()?;
println!("1) {:?}", port_info);
drop(camera);
println!("2) {:?}", port_info);

1) PortInfo { name: "Universal Serial Bus", path: "usb:001,001", port_type: Some(Usb) }
2) PortInfo { name: "pG�m�U", path: "��m�U", port_type: Some(Usb) }

PortInfo should probably hold reference to Camera (either as a Rust reference, or via libgphoto2's reference counting), but also, where is one there might be others.

It's worth walking through other data structures that have pointers and are returned from camera / context / filesystem / etc. and making sure they are not tied similarly to their parents.

[RReverser]

After a quick glance at sources of this and other types, I think this should be the only problematic one, because it's declared as

typedef struct _GPPortInfo *GPPortInfo;

and not just

typedef struct _SomeStruct SomeStruct;

as others are.

That is, we store a pointer to a pointer, in which case it makes sense that it can become invalid. But maybe there are more, another pair of eyes would be helpful...

[maxicarlos08]

I just took a quick look at libgphoto2 and only found the typedef struct _GPPortInfo *GPPortInfo.

If I have time I will see if I find any others

[maxicarlos08]

@RReverser Have you found any other structures with the same issue? If not I will close this for now.

BTW: People are asking me when the next release will be, are there still any breaking changes you want to do? Otherwise I think I will release today

[RReverser]

Have you found any other structures with the same issue?

I haven't, but I haven't really looked for more as you said you would.

BTW: People are asking me when the next release will be, are there still any breaking changes you want to do?

One big question mark I have in mind is still dealing with strings:

1. Are there places we can still make Cow safely instead of String, or whether we should just return owned String everywhere and
2. I've been looking a bit into how libgphoto2 deals with encodings, and found that it actually respects system localisation + some undocumented APIs for overriding localisation/encodings. What worries me there is that I'm not certain that non-English strings are guaranteed to be encoded in UTF-8 by libgphoto2, in which case perhaps we don't have a right to return them as String, or perhaps we at least have to customize encoding upon startup to enforce UTF-8 or something... As I said, I only started looking at this and there's a bit of uncertainty here. Now that I added test framework, I think it would be helpful to try and verify those assumptions and check what happens with non-ASCII characters.

If you're in rush, I suppose you can make a release as-is, but if not, I'd rather ask you to investigate the encoding stuff a bit and tell people they can use https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html to depend on the Github repo directly for now (and you can do the same in your gcam project).

[maxicarlos08]

Ok, then I'll wait until we fix those two points

to depend on the Github repo directly for now (and you can do the same in your gcam project).

I did that in the past when I was doing some experimentation with this crate but gcam has fallen asleep lately, I hope I can continue worknig on it soon

cc @gKodes Sorry, the release wont be today then, you can set your dependency to:

gphoto2 = { git = "https://github.com/maxicarlos08/gphoto2-rs.git" }

for now to get the git version

[RReverser]

I mean, it won't be worse than the current published version at least, just saying if you release 2.0, then we might have to do 2.1 again soon :)

[gKodes]

@maxicarlos08 i am already using the git version. No worries, just was checking in as there we quite a no of issues resolved.

[RReverser]

I guess this particular issue can be closed until we find evidence to the contrary.