search

quickwit-oss / tantivy

Tantivy is a full-text search engine library inspired by Apache Lucene and written in Rust
MIT License
12.09k stars 670 forks source link

Remove dependency atomicwrites #833

Closed asafigan closed 4 years ago

asafigan commented 4 years ago

From atomicwrites' readme:

Alternatives

  • tempfile has a persist method doing the same thing.

We are already using tempfile so we could remove the dependency on atomicwrite. We are only using it in one method so it's not a large change.

I am willing to make a PR for this.

fulmicoton commented 4 years ago

@asafigan this sounds like a good idea

asafigan commented 4 years ago

I closed it because after looking into persist, it not a one to one replacement. There are subtle implementation details that I don't understand.

One of the main differences is that atomicwrites creates a temporary directory to put a temporary file in. Where as persist would just have a single temporary file.

When I asked why it is done, I didn't really get a good answer back. It may or may not prevent some types of attacks. https://github.com/untitaker/rust-atomicwrites/pull/40

I don't see how TOCTOU would be a problem but security issues are usually subtle and I don't know much about filesystems.

There are also subtly different syscalls which I don't know the differences.

I also don't know if these are concerns for tantivy since our use case is so simple.

If these differences are not a concern, we can move forward. If they are, we would have to invest time into understanding them before moving forward. I felt wasn't worth the time, which is why I closed the issue.

fulmicoton commented 4 years ago

@asafigan There is some description of the possible issue in the documentation of the persist method. I think this is ok for tantivy, so I opened a PR accordingly.