[PATCH libXpm] After fdopen(), use fclose() instead of close() in error path

[Alan Coopersmith]

Found by Oracle's Parfait 2.2 static analyzer:

Error: File Leak
File Leak [file-ptr-leak]:
Leaked File fp
at line 94 of lib/libXpm/src/RdFToBuf.c in function 'XpmReadFileToBuffer
'.
fp initialized at line 86 with fdopen
fp leaks when len < 0 at line 92.

Introduced-by: commit 8b3024e6871ce50b34bf2dff924774bd654703bc

Signed-off-by: Alan Coopersmith <***@oracle.com>
---
src/RdFToBuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/RdFToBuf.c b/src/RdFToBuf.c
index 69e3347..1b386f8 100644
--- a/src/RdFToBuf.c
+++ b/src/RdFToBuf.c
@@ -86,15 +86,15 @@ XpmReadFileToBuffer(
fp = fdopen(fd, "r");
if (!fp) {
close(fd);
return XpmOpenFailed;
}
len = stats.st_size;
if (len < 0 || len >= SIZE_MAX) {
- close(fd);
+ fclose(fp);
return XpmOpenFailed;
}
ptr = (char *) XpmMalloc(len + 1);
if (!ptr) {
fclose(fp);
return XpmNoMemory;
}

[Peter Hutterer]

Post by Alan Coopersmith
Error: File Leak
Leaked File fp
at line 94 of lib/libXpm/src/RdFToBuf.c in function 'XpmReadFileToBuffer
'.
fp initialized at line 86 with fdopen
fp leaks when len < 0 at line 92.
Introduced-by: commit 8b3024e6871ce50b34bf2dff924774bd654703bc

Reviewed-by: Peter Hutterer <***@who-t.net>

Cheers,
Peter

Post by Alan Coopersmith
---
src/RdFToBuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/RdFToBuf.c b/src/RdFToBuf.c
index 69e3347..1b386f8 100644
--- a/src/RdFToBuf.c
+++ b/src/RdFToBuf.c
@@ -86,15 +86,15 @@ XpmReadFileToBuffer(
fp = fdopen(fd, "r");
if (!fp) {
close(fd);
return XpmOpenFailed;
}
len = stats.st_size;
if (len < 0 || len >= SIZE_MAX) {
- close(fd);
+ fclose(fp);
return XpmOpenFailed;
}
ptr = (char *) XpmMalloc(len + 1);
if (!ptr) {
fclose(fp);
return XpmNoMemory;
}
--
2.15.2
_______________________________________________
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
In

[Walter Harms]

Post by Alan Coopersmith
Error: File Leak
Leaked File fp
at line 94 of lib/libXpm/src/RdFToBuf.c in function
'XpmReadFileToBuffer
'.
fp initialized at line 86 with fdopen
fp leaks when len < 0 at line 92.
Introduced-by: commit 8b3024e6871ce50b34bf2dff924774bd654703bc
---
src/RdFToBuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/RdFToBuf.c b/src/RdFToBuf.c
index 69e3347..1b386f8 100644
--- a/src/RdFToBuf.c
+++ b/src/RdFToBuf.c
@@ -86,15 +86,15 @@ XpmReadFileToBuffer(
fp = fdopen(fd, "r");
if (!fp) {
close(fd);
return XpmOpenFailed;
}
len = stats.st_size;
if (len < 0 || len >= SIZE_MAX) {
- close(fd);
+ fclose(fp);
return XpmOpenFailed;
}

IMHO it should do both,
otherwise you have a different behavier when
fdopen failed and returning XpmOpenFailed
or size check failed and returning XpmOpenFailed

Post by Alan Coopersmith
ptr = (char *) XpmMalloc(len + 1);
if (!ptr) {
fclose(fp);
return XpmNoMemory;
}

this does not close(fd) either, maybe
it is better not to close it in the first case
to have a similar behavier ?

re,
wh

Post by Alan Coopersmith
--
2.15.2
_______________________________________________
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailm

[Alan Coopersmith]

Post by Walter Harms

Post by Alan Coopersmith
Error: File Leak
Leaked File fp
at line 94 of lib/libXpm/src/RdFToBuf.c in function
'XpmReadFileToBuffer
'.
fp initialized at line 86 with fdopen
fp leaks when len < 0 at line 92.
Introduced-by: commit 8b3024e6871ce50b34bf2dff924774bd654703bc
---
src/RdFToBuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/RdFToBuf.c b/src/RdFToBuf.c
index 69e3347..1b386f8 100644
--- a/src/RdFToBuf.c
+++ b/src/RdFToBuf.c
@@ -86,15 +86,15 @@ XpmReadFileToBuffer(
fp = fdopen(fd, "r");
if (!fp) {
close(fd);
return XpmOpenFailed;
}
len = stats.st_size;
if (len < 0 || len >= SIZE_MAX) {
- close(fd);
+ fclose(fp);
return XpmOpenFailed;
}

IMHO it should do both,
otherwise you have a different behavier when
fdopen failed and returning XpmOpenFailed
or size check failed and returning XpmOpenFailed

Once fp = fdopen(fd, ...) succeeds, then fclose(fp) will also close the
underlying fd, so us trying to close it again will cause EBADF errors in
most cases, or possible race conditions if libXpm is being used in a
multi-threaded program that opens another fd in another thread at the
same time.