Re: BUG #15105: OpenTransientFile() should be paired with CloseTransientFile() rather than close()

The following bug has been logged on the website:

Bug reference: 15105
Logged by: Pan Bian
Email address: bianpan2016(at)163(dot)com
PostgreSQL version: 10.3
Operating system: Linux
Description:

File: src/backend/storage/ipc/dsm_impl.c
Function: dsm_impl_mmap()

Details: The handler opened with OpenTransientFile() should be closed with
CloseTransientFile(). However, in function dsm_impl_mmap(), on a certain
path, the return value of OpenTransientFile() (at line 885) is passed to
close() (at line 926). It is better to use CloseTransientFile() here, as
done at line 909.

For your convenience, I paste the related source code as follows:

845 static bool
846 dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
847 void **impl_private, void **mapped_address, Size
*mapped_size,
848 int elevel)
849 {
...
883 /* Create new segment or open an existing one for attach or resize.
*/
884 flags = O_RDWR | (op == DSM_OP_CREATE ? O_CREAT | O_EXCL : 0);
885 if ((fd = OpenTransientFile(name, flags, 0600)) == -1)
886 {
887 if (errno != EEXIST)
888 ereport(elevel,
889 (errcode_for_dynamic_shared_memory(),
890 errmsg("could not open shared memory segment
\"%s\": %m",
891 name)));
892 return false;
893 }
894
895 /*
896 * If we're attaching the segment, determine the current size; if
we are
897 * creating or resizing the segment, set the size to the requested
value.
898 */
899 if (op == DSM_OP_ATTACH)
900 {
901 struct stat st;
902
903 if (fstat(fd, &st) != 0)
904 {
905 int save_errno;
906
907 /* Back out what's already been done. */
908 save_errno = errno;
909 CloseTransientFile(fd);
910 errno = save_errno;
911
912 ereport(elevel,
913 (errcode_for_dynamic_shared_memory(),
914 errmsg("could not stat shared memory segment
\"%s\": %m",
915 name)));
916 return false;
917 }
918 request_size = st.st_size;
919 }
920 else if (*mapped_size > request_size && ftruncate(fd,
request_size))
921 {
922 int save_errno;
923
924 /* Back out what's already been done. */
925 save_errno = errno;
926 close(fd);
927 if (op == DSM_OP_CREATE)
928 unlink(name);
929 errno = save_errno;
930
931 ereport(elevel,
932 (errcode_for_dynamic_shared_memory(),
933 errmsg("could not resize shared memory segment \"%s\"
to %zu bytes: %m",
934 name, request_size)));
935 return false;
936 }
...
1038 *mapped_address = address;
1039 *mapped_size = request_size;
1040 CloseTransientFile(fd);
1041
1042 return true;
1043 }

Thanks!

On Fri, Mar 09, 2018 at 03:29:25AM +0000, PG Bug reporting form wrote:
> Details: The handler opened with OpenTransientFile() should be closed with
> CloseTransientFile(). However, in function dsm_impl_mmap(), on a certain
> path, the return value of OpenTransientFile() (at line 885) is passed to
> close() (at line 926). It is better to use CloseTransientFile() here, as
> done at line 909.

Good catch! This is visibly a copy-paste error coming from
dsm_impl_posix(). As a patch it gives the attached. I am adding also
Robert in CC for awareness.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> On Fri, Mar 09, 2018 at 03:29:25AM +0000, PG Bug reporting form wrote:
>> Details: The handler opened with OpenTransientFile() should be closed with
>> CloseTransientFile(). However, in function dsm_impl_mmap(), on a certain
>> path, the return value of OpenTransientFile() (at line 885) is passed to
>> close() (at line 926). It is better to use CloseTransientFile() here, as
>> done at line 909.

> Good catch! This is visibly a copy-paste error coming from
> dsm_impl_posix(). As a patch it gives the attached. I am adding also
> Robert in CC for awareness.

Presumably, this would have been found sooner if AtEOXact_Files acted
like most other end-of-xact cleanup functions and whined about leaked
resources (in the commit case). I wonder why it doesn't.

regards, tom lane

I wrote:
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>> On Fri, Mar 09, 2018 at 03:29:25AM +0000, PG Bug reporting form wrote:
>>> Details: The handler opened with OpenTransientFile() should be closed with
>>> CloseTransientFile(). However, in function dsm_impl_mmap(), on a certain
>>> path, the return value of OpenTransientFile() (at line 885) is passed to
>>> close() (at line 926). It is better to use CloseTransientFile() here, as
>>> done at line 909.

>> Good catch! This is visibly a copy-paste error coming from
>> dsm_impl_posix(). As a patch it gives the attached. I am adding also
>> Robert in CC for awareness.

Now that the commitfest crunch is over, I've checked and pushed this.

> Presumably, this would have been found sooner if AtEOXact_Files acted
> like most other end-of-xact cleanup functions and whined about leaked
> resources (in the commit case). I wonder why it doesn't.

On closer inspection, such a cross-check wouldn't have helped find this
particular mistake, because it was in an error exit path that's likely
never been exercised by anybody, and certainly isn't hit in the regression
tests. Still, I think it'd be a good thing to add, and hence propose the
attached patch. (I've verified that the warning doesn't fire in "make
check-world".)

regards, tom lane

On Tue, Apr 10, 2018 at 06:40:43PM -0400, Tom Lane wrote:
> On closer inspection, such a cross-check wouldn't have helped find this
> particular mistake, because it was in an error exit path that's likely
> never been exercised by anybody, and certainly isn't hit in the regression
> tests. Still, I think it'd be a good thing to add, and hence propose the
> attached patch. (I've verified that the warning doesn't fire in "make
> check-world".)

Good idea. +1 for your proposed patch.
--
Michael