Hi!
On Fri, 2011-04-01 at 15:12:46 -0500, Jonathan Nieder wrote:
> Without this change, dpkg-divert might write
>
> cannot rename 'src' to 'dest': Success
>
> because close-ing the src file descriptor to clean up can clobber
> errno after creat has failed. (This probably does not happen with
> glibc unless the "close" fails, but it seems best to be safe.)
Right, which is worse than "leaking" the file descriptor, in this case
a non-issue as the program is short-lived anyway, and the code will
exit immediately on return -1. The only purpose of closing the leak here
is to shutup cppcheck, which while I think is good in general, it might
imply the code needs reestructuring. Or sometimes it's just not worth
it if it uglifies it. The same applies to warnings. :)
I was aware of the "leaking" issue, and I actually have a fix stashed
(should probably stop using the stash so much as it's not really
visible :/), but was not planning on acting on the bug report for
1.16.0. But I think I'll revert Raphaël's change and apply my stashed
changes.
The current patch missed too the case that if fsync() fails it leaks
dstfd.
> diff --git a/src/divertcmd.c b/src/divertcmd.c
> index e01ece0..def2071 100644
> --- a/src/divertcmd.c
> +++ b/src/divertcmd.c
> @@ -217,7 +217,9 @@ file_copy(const char *src, const char *dst)
>
> dstfd = creat(dst, 0600);
> if (dstfd < 0) {
> + int e = errno;
> close(srcfd);
> + errno = e;
> return -1;
> }
I'd rather not do stuff like this for general error unwinding if not
strictly necessary. :)
thanks,
guillem
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
