IndexEntry path and flags not safe to expose directly

[joshtriplett]

In the underlying git_index_entry structure, the low 12 bits of flags define the length of the path (with 0xFFF indicating a need to search for a NUL terminator, which as far as I can tell must always exist for an in-memory structure regardless of length). IndexEntry exposes the path and flags fields directly, without handling NUL termination or length. An IndexEntry constructed in safe code can cause libgit2 to access memory it shouldn't and write that out as part of the path. For instance, I constructed an IndexEntry for a file "base", and the index on disk ended up with a file "basec6d", where "c6d" were characters of a recently handled short hash ID.

git2-rs needs to provide a safer wrapper for IndexEntry, making the safer fields available directly, but providing wrappers for the path, stage, and flags, to maintain consistency. Since IndexEntry won't have all of its fields available, it will also need an appropriate constructor. I'd suggest an IndexEntry::new that takes a Path, mode, and Option<Oid>.

[alexcrichton]

Oh dear I had no idea!

[joshtriplett]

Investigating more carefully, it looks like libgit2 might handle the length automatically, at least in git_index_add. But it's still not safe to expose path directly, because it must be NUL-terminated.

[alexcrichton]

Yeah this sounds pretty bad all around, and IndexEntry may end up just needing a complete overhaul

[joshtriplett]

A related issue I just ran into: if you read an IndexEntry using git2::Index::get_path, and then add that entry to another index using .git2::Index::add, that can read off the end of the .path field. Whatever safety property .add needs, the IndexEntry returned by .get_path doesn't seem to satisfy.

[alexcrichton]

Oh dear, this appears to be extra bad then! Thanks for the reports!

Ah and I am indeed acrichto on IRC, sorry I missed your ping! I probably won't be able to get around to this until perhaps this weekend at the earliest, but if you want to try to tackle it ahead of time feel free as well!

[alexcrichton]

Ok, can you try giving master a spin? If it works I'll publish a release

[joshtriplett]

@alexcrichton Seems to work; I removed the workarounds I had for NUL handling, and everything seems functional.

I made one comment on the commit, regarding whether IndexEntry should have a Path instead of a Vec<u8> to make it explicit that it shouldn't have NUL handling. Otherwise, this looks good to me.

[alexcrichton]

Ok, thanks for checking!