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

2018-10-11 Thread Alan Coopersmith 

On 10/11/18 12:13 PM, Walter Harms wrote:




Alan Coopersmith  hat am 1. Oktober 2018 um 00:14
geschrieben:


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 
---
  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.

--
-Alan Coopersmith-   alan.coopersm...@oracle.com
 Oracle Solaris Engineering - https://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

2018-10-11 Thread Walter Harms 


> Alan Coopersmith  hat am 1. Oktober 2018 um 00:14
> geschrieben:
> 
> 
> 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 
> ---
>  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



>  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


> -- 
> 2.15.2
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

2018-10-01 Thread Peter Hutterer 
On Sun, Sep 30, 2018 at 03:14:00PM -0700, Alan Coopersmith wrote:
> 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 

Reviewed-by: Peter Hutterer 

Cheers,
   Peter

> ---
>  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
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

2018-09-30 Thread 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 
---
 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

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel