Bug: SIGABRT if "notmuch dump" output file is not writeable

classic Classic list List threaded Threaded
6 messages Options
Ralph Seichter-2 Ralph Seichter-2
Reply | Threaded
Open this post in threaded view
|

Bug: SIGABRT if "notmuch dump" output file is not writeable

This is what happens when calling "notmuch dump" (version 0.29.1) with
an output file that is not writeable (tested with FISH and BASH):

  root > touch /tmp/out
  root > ls -l /tmp/out
  -rw-r--r-- 1 root root 0 Jul 22 20:36 /tmp/out

  nonroot > notmuch dump --output=/tmp/out
  Error renaming /tmp/out.kuZ9t5 to /tmp/out: Operation not permitted
  double free or corruption (!prev)
  fish: “notmuch dump --output=/tmp/out” terminated by signal SIGABRT (Abort)

While it is understood that Notmuch cannot write to the specified output
file, I don't think this should result in something as harsh as SIGABRT.

-Ralph
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: Bug: SIGABRT if "notmuch dump" output file is not writeable

Ralph Seichter <[hidden email]> writes:

> While it is understood that Notmuch cannot write to the specified output
> file, I don't think this should result in something as harsh as SIGABRT.

I agree it's a bit ugly to look at. At least it prints what the actual
problem is before the abort. The abort seems to be generated from
gzclose_w, so I guess it might just be an error handling bug in libz.

Do you see any database corruption or more serious issues?

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Ralph Seichter-2 Ralph Seichter-2
Reply | Threaded
Open this post in threaded view
|

Re: Bug: SIGABRT if "notmuch dump" output file is not writeable

* David Bremner:

> I agree it's a bit ugly to look at.

Ah, euphemisms. ;-) Personally, I associate "double free or corruption
(!prev)" with memory trouble or situations where a library cannot
recover from an error state and needs to bail out using abort(). Not
being able to (over)write an existing file is not that serious, IMO.

> Do you see any database corruption or more serious issues?

No, and I don't expect any, as I am assuming that "notmuch dump" will
only ever read the DB.

-Ralph
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: Bug: SIGABRT if "notmuch dump" output file is not writeable

Ralph Seichter <[hidden email]> writes:

> * David Bremner:
>
>> I agree it's a bit ugly to look at.
>
> Ah, euphemisms. ;-) Personally, I associate "double free or corruption
> (!prev)" with memory trouble or situations where a library cannot
> recover from an error state and needs to bail out using abort(). Not
> being able to (over)write an existing file is not that serious, IMO.

Yes, but that's a message / abort from deep within libz. So odds of our
being able to fix it are pretty small. Checking for permissions before
hand would just introduce a race condition (I _think_).

>
>> Do you see any database corruption or more serious issues?
>
> No, and I don't expect any, as I am assuming that "notmuch dump" will
> only ever read the DB.

That's true, but it does take write lock on the database so that
one dumps a consistent state.

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Ralph Seichter-2 Ralph Seichter-2
Reply | Threaded
Open this post in threaded view
|

Re: Bug: SIGABRT if "notmuch dump" output file is not writeable

* David Bremner:

> that's a message / abort from deep within libz. So odds of our being
> able to fix it are pretty small.

Based on a quick glance on notmuch_database_dump() in notmuch-dump.c, it
seems to me that SIGABRT occurs in line 351:

  350: if (ret != EXIT_SUCCESS && output)
  351:   (void) gzclose_w (output);

gzclose_w(output) has already been called in line 332, before Notmuch
attempts to rename the temp file to the output file. At that point,
'output' should be set to null as it is being checked later, but that
erroneously only happens in case the close operation fails.

The rename in line 341 fails because of permission/ownership issues,
'ret' contains the error code for that, 'output' is still non-null, so
gzclose_w is called again -- ergo boom.

I have not tested or debugged this so far, it is just a Mark-I-Eyeball
analysis. I think I got it right, though. If you agree, I can provide a
fix, which I will actually test.

-Ralph
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: Bug: SIGABRT if "notmuch dump" output file is not writeable

Ralph Seichter <[hidden email]> writes:

> gzclose_w(output) has already been called in line 332, before Notmuch
> attempts to rename the temp file to the output file. At that point,
> 'output' should be set to null as it is being checked later, but that
> erroneously only happens in case the close operation fails.
>
> The rename in line 341 fails because of permission/ownership issues,
> 'ret' contains the error code for that, 'output' is still non-null, so
> gzclose_w is called again -- ergo boom.

That sounds plausible to me. Thanks for thinking about this longer than
I did. I'll be happy to look at a patch.

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch