Thread: Memory leak with XLogFileCopy since de768844 (WAL file with .partial)

Hi all,

Since commit de768844, XLogFileCopy of xlog.c returns to caller a
pstrdup'd string that can be used afterwards for other things.
XLogFileCopy is used in only one place, and it happens that the result
string is never freed at all, leaking memory.
Attached is a patch to fix the problem.
Regards,
--
Michael

On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Since commit de768844, XLogFileCopy of xlog.c returns to caller a
> pstrdup'd string that can be used afterwards for other things.
> XLogFileCopy is used in only one place, and it happens that the result
> string is never freed at all, leaking memory.
> Attached is a patch to fix the problem.

Having a second look at that, XLogFileCopy() is called only in one
place, and dstfname is never used, hence I think that it would make
sense to return unconditionally a temporary file name to the caller.
Also the current error message in case of failure is very C-like:
elog(ERROR, "InstallXLogFileSegment should not have failed");
I thought that we to use function names in the error messages.
Wouldn't something like that be more adapted?
"could not copy partial log file" or ""could not copy partial log file
into temporary segment file"
Attached is a new patch.

Regards,
--
Michael

On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Since commit de768844, XLogFileCopy of xlog.c returns to caller a
>> pstrdup'd string that can be used afterwards for other things.
>> XLogFileCopy is used in only one place, and it happens that the result
>> string is never freed at all, leaking memory.

That seems to be almost harmless because the startup process will exit
just after calling XLogFileCopy(). No?

> Having a second look at that, XLogFileCopy() is called only in one
> place, and dstfname is never used, hence I think that it would make
> sense to return unconditionally a temporary file name to the caller.

+1

> Also the current error message in case of failure is very C-like:
> elog(ERROR, "InstallXLogFileSegment should not have failed");
> I thought that we to use function names in the error messages.
> Wouldn't something like that be more adapted?
> "could not copy partial log file" or ""could not copy partial log file
> into temporary segment file"

Or we can extend InstallXLogFileSegment so that we can give the log level
to it. When InstallXLogFileSegment is called after XLogFileCopy, we can
give ERROR as the log level and cause an exception to occur if link() or
rename() fails in InstallXLogFileSegment.

Regards,

-- 
Fujii Masao

On Fri, Jun 5, 2015 at 12:39 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>
>
> On Thu, Jun 4, 2015 at 10:40 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>> > On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
>> > <michael.paquier@gmail.com> wrote:
>> >> Since commit de768844, XLogFileCopy of xlog.c returns to caller a
>> >> pstrdup'd string that can be used afterwards for other things.
>> >> XLogFileCopy is used in only one place, and it happens that the result
>> >> string is never freed at all, leaking memory.
>>
>> That seems to be almost harmless because the startup process will exit
>> just after calling XLogFileCopy(). No?
>
>
> Yes that's harmless. My point here is correctness, prevention does not hurt
> particularly if this code path is used more in the future.

Why don't we call InstallXLogFileSegment() at the end of XLogFileCopy()?
If we do that, the risk of memory leak you're worried will disappear at all.

Regards,

-- 
Fujii Masao

On Fri, Jun 5, 2015 at 10:45 PM, Fujii Masao wrote:
> Why don't we call InstallXLogFileSegment() at the end of XLogFileCopy()?
> If we do that, the risk of memory leak you're worried will disappear at all.

Yes, that looks fine, XLogFileCopy() would copy to a temporary file,
then install it definitely. Updated patch attached.
--
Michael

On Mon, Jun 8, 2015 at 11:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Jun 5, 2015 at 10:45 PM, Fujii Masao wrote:
>> Why don't we call InstallXLogFileSegment() at the end of XLogFileCopy()?
>> If we do that, the risk of memory leak you're worried will disappear at all.
>
> Yes, that looks fine, XLogFileCopy() would copy to a temporary file,
> then install it definitely. Updated patch attached.

Thanks for updating the patch! Looks good to me. Applied.

Regards,

-- 
Fujii Masao

On 06/08/2015 09:04 PM, Fujii Masao wrote:
> On Mon, Jun 8, 2015 at 11:52 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Jun 5, 2015 at 10:45 PM, Fujii Masao wrote:
>>> Why don't we call InstallXLogFileSegment() at the end of XLogFileCopy()?
>>> If we do that, the risk of memory leak you're worried will disappear at all.
>>
>> Yes, that looks fine, XLogFileCopy() would copy to a temporary file,
>> then install it definitely. Updated patch attached.
>
> Thanks for updating the patch! Looks good to me. Applied.

XLogFileCopy() used to call InstallXLogFileSegment(), until I refactored 
that in commit de7688442f5aaa03da60416a6aa3474738718803. That commit 
added another caller of XLogFileCopy(), which didn't want to install the 
segment. However, I later partially reverted that patch in commit 
7cbee7c0a1db668c60c020a3fd1e3234daa562a9, and those changes to 
XLogFileCopy() were not really needed anymore. I decided not to revert 
those changes at that point because that refactoring seemed sensible 
anyway. See my email at 
http://www.postgresql.org/message-id/555DD101.7080209@iki.fi about that.

I'm still not sure if I should've just reverted that refactoring, to 
make XLogFileCopy() look the same in master and back-branches, which 
makes back-patching easier, or keep the refactoring, because it makes 
the code slightly nicer. But the current situation is the worst of both 
worlds: the interface of XLogFileCopy() is no better than it used to be, 
but it's different enough to cause merge conflicts. At this point, it's 
probably best to revert the code to look the same as in 9.4.

- Heikki

On Tue, Jun 9, 2015 at 6:25 AM, Heikki Linnakangas wrote:
> I'm still not sure if I should've just reverted that refactoring, to make
> XLogFileCopy() look the same in master and back-branches, which makes
> back-patching easier, or keep the refactoring, because it makes the code
> slightly nicer. But the current situation is the worst of both worlds: the
> interface of XLogFileCopy() is no better than it used to be, but it's
> different enough to cause merge conflicts. At this point, it's probably best
> to revert the code to look the same as in 9.4.

That's a valid concern. What about the attached then? I think that it
is still good to keep upto to copy only data up to the switch point at
recovery exit. InstallXLogFileSegment() changes a bit as well because
of its modifications of arguments.
--
Michael

On Tue, Jun 9, 2015 at 6:25 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 06/08/2015 09:04 PM, Fujii Masao wrote:
>>
>> On Mon, Jun 8, 2015 at 11:52 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>>
>>> On Fri, Jun 5, 2015 at 10:45 PM, Fujii Masao wrote:
>>>>
>>>> Why don't we call InstallXLogFileSegment() at the end of XLogFileCopy()?
>>>> If we do that, the risk of memory leak you're worried will disappear at
>>>> all.
>>>
>>>
>>> Yes, that looks fine, XLogFileCopy() would copy to a temporary file,
>>> then install it definitely. Updated patch attached.
>>
>>
>> Thanks for updating the patch! Looks good to me. Applied.
>
>
> XLogFileCopy() used to call InstallXLogFileSegment(), until I refactored
> that in commit de7688442f5aaa03da60416a6aa3474738718803. That commit added
> another caller of XLogFileCopy(), which didn't want to install the segment.
> However, I later partially reverted that patch in commit
> 7cbee7c0a1db668c60c020a3fd1e3234daa562a9, and those changes to
> XLogFileCopy() were not really needed anymore. I decided not to revert those
> changes at that point because that refactoring seemed sensible anyway. See
> my email at http://www.postgresql.org/message-id/555DD101.7080209@iki.fi
> about that.
>
> I'm still not sure if I should've just reverted that refactoring, to make
> XLogFileCopy() look the same in master and back-branches, which makes
> back-patching easier, or keep the refactoring, because it makes the code
> slightly nicer. But the current situation is the worst of both worlds: the
> interface of XLogFileCopy() is no better than it used to be, but it's
> different enough to cause merge conflicts. At this point, it's probably best
> to revert the code to look the same as in 9.4.

I'm not sure how much it's really nicer to keep the refactoring,
but I agree that it's better to make the code look same to make
the back-patching easier.

Regards,

-- 
Fujii Masao

On Tue, Jun 9, 2015 at 10:09 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jun 9, 2015 at 6:25 AM, Heikki Linnakangas wrote:
>> I'm still not sure if I should've just reverted that refactoring, to make
>> XLogFileCopy() look the same in master and back-branches, which makes
>> back-patching easier, or keep the refactoring, because it makes the code
>> slightly nicer. But the current situation is the worst of both worlds: the
>> interface of XLogFileCopy() is no better than it used to be, but it's
>> different enough to cause merge conflicts. At this point, it's probably best
>> to revert the code to look the same as in 9.4.
>
> That's a valid concern. What about the attached then? I think that it
> is still good to keep upto to copy only data up to the switch point at
> recovery exit. InstallXLogFileSegment() changes a bit as well because
> of its modifications of arguments.

Applied. Thanks!

Regards,

-- 
Fujii Masao

On Wed, Jul 1, 2015 at 10:58 AM, Fujii Masao wrote:
> On Tue, Jun 9, 2015 at 10:09 AM, Michael Paquier wrote:
>> That's a valid concern. What about the attached then? I think that it
>> is still good to keep upto to copy only data up to the switch point at
>> recovery exit. InstallXLogFileSegment() changes a bit as well because
>> of its modifications of arguments.
>
> Applied. Thanks!

Thanks for showing up.
-- 
Michael