[PATCH 0/4] Fix mmap return checking

[PATCH 0/4] Fix mmap return checking

[Don Slutz]

On Linux MAP_FAILED is not the same as NULL.

Since lots of other calls check for MAP_FAILED, fix to use
MAP_FAILED.

tools/blktap/drivers/tapdisk.c does check for -1 not MAP_FAILED.  I
did not change since it was not clearly wrong.


Don Slutz (3):
  loadpolicy.c: Fix return handling for case of mmap failure
  libxl_internal.c: Fix return handling for case of mmap failure
  xenbaked.c: Fix return handling for case of mmap failure

Harry Hart (1):
  libxc/xc_linux_osdep.c: Fix return handling for case of mmap failure

 tools/flask/utils/loadpolicy.c | 2 +-
 tools/libxc/xc_linux_osdep.c   | 5 +++++
 tools/libxl/libxl_internal.c   | 2 +-
 tools/xenmon/xenbaked.c        | 2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)

-- 
1.8.4

[PATCH 1/4] libxc/xc_linux_osdep.c: Fix return handling for case of mmap failure

[Don Slutz]

From: Harry Hart <hhart@verizon.com>

Callers expect NULL on errors not MAP_FAILED.

Signed-off-by: Harry Hart <hhart@verizon.com>
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/libxc/xc_linux_osdep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index 86bff3e..9745feb 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -102,6 +102,11 @@ static void *linux_privcmd_alloc_hypercall_buffer(xc_interface *xch, xc_osdep_ha
         return NULL;
     }
 
+    if (p == MAP_FAILED) {
+        PERROR ("linux_privcmd_alloc_hypercall_buffer: mmap failed");
+        return NULL;
+    }
+
     /* Do not copy the VMA to child process on fork. Avoid the page being COW
         on hypercall. */
     rc = madvise(p, npages * XC_PAGE_SIZE, MADV_DONTFORK);
-- 
1.8.4

Re: [PATCH 1/4] libxc/xc_linux_osdep.c: Fix return handling for case of mmap failure

[Ian Campbell]

On Mon, 2014-07-28 at 12:05 -0400, Don Slutz wrote:
> From: Harry Hart <hhart@verizon.com>
> 
> Callers expect NULL on errors not MAP_FAILED.
> 
> Signed-off-by: Harry Hart <hhart@verizon.com>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  tools/libxc/xc_linux_osdep.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> index 86bff3e..9745feb 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -102,6 +102,11 @@ static void *linux_privcmd_alloc_hypercall_buffer(xc_interface *xch, xc_osdep_ha
>          return NULL;
>      }
>  
> +    if (p == MAP_FAILED) {
> +        PERROR ("linux_privcmd_alloc_hypercall_buffer: mmap failed");
> +        return NULL;
> +    }

The return NULL just above in the context is the same check as this.
Rebasing mistake?

Re: [PATCH 1/4] libxc/xc_linux_osdep.c: Fix return handling for case of mmap failure

[Don Slutz]

On 07/30/14 07:42, Ian Campbell wrote:
> On Mon, 2014-07-28 at 12:05 -0400, Don Slutz wrote:
>> From: Harry Hart <hhart@verizon.com>
>>
>> Callers expect NULL on errors not MAP_FAILED.
>>
>> Signed-off-by: Harry Hart <hhart@verizon.com>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   tools/libxc/xc_linux_osdep.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
>> index 86bff3e..9745feb 100644
>> --- a/tools/libxc/xc_linux_osdep.c
>> +++ b/tools/libxc/xc_linux_osdep.c
>> @@ -102,6 +102,11 @@ static void *linux_privcmd_alloc_hypercall_buffer(xc_interface *xch, xc_osdep_ha
>>           return NULL;
>>       }
>>   
>> +    if (p == MAP_FAILED) {
>> +        PERROR ("linux_privcmd_alloc_hypercall_buffer: mmap failed");
>> +        return NULL;
>> +    }
> The return NULL just above in the context is the same check as this.
> Rebasing mistake?
>
>
>

Yup.

    -Don Slutz

[PATCH 2/4] loadpolicy.c: Fix return handling for case of mmap failure

[Don Slutz]

mmap() returns MAP_FAILED not NULL.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/flask/utils/loadpolicy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/flask/utils/loadpolicy.c b/tools/flask/utils/loadpolicy.c
index 2edcf97..47b5139 100644
--- a/tools/flask/utils/loadpolicy.c
+++ b/tools/flask/utils/loadpolicy.c
@@ -61,7 +61,7 @@ int main (int argCnt, const char *args[])
 
 #ifdef USE_MMAP
     polMem = mmap(NULL, info.st_size, PROT_READ, MAP_SHARED, polFd, 0);
-    if ( !polMem )
+    if ( polMem == MAP_FAILED )
     {
         fprintf(stderr, "Error occurred mapping policy file in memory: %s\n",
                 strerror(errno));
-- 
1.8.4

[PATCH 3/4] libxl_internal.c: Fix return handling for case of mmap failure

[Don Slutz]

mmap() returns MAP_FAILED not NULL.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/libxl/libxl_internal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 81f8985..b880c89 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -246,7 +246,7 @@ int libxl__file_reference_map(libxl__file_reference *f)
 
     ret = -1;
     data = mmap(NULL, st_buf.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-    if (data == NULL)
+    if (data == MAP_FAILED)
         goto out;
 
     f->mapped = 1;
-- 
1.8.4

[PATCH 4/4] xenbaked.c: Fix return handling for case of mmap failure

[Don Slutz]

mmap() returns MAP_FAILED not NULL.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/xenmon/xenbaked.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenmon/xenbaked.c b/tools/xenmon/xenbaked.c
index 985f1dd..dc61d14 100644
--- a/tools/xenmon/xenbaked.c
+++ b/tools/xenmon/xenbaked.c
@@ -674,7 +674,7 @@ static void alloc_qos_data(int ncpu)
         new_qos = (_new_qos_data *) mmap(0, sizeof(_new_qos_data), PROT_READ|PROT_WRITE, 
                                          MAP_SHARED, qos_fd, off);
         off += i;
-        if (new_qos == NULL) {
+        if (new_qos == MAP_FAILED) {
             PERROR("mmap");
             exit(3);
         }
-- 
1.8.4

Re: [PATCH 0/4] Fix mmap return checking

[Ian Campbell]

On Mon, 2014-07-28 at 12:05 -0400, Don Slutz wrote:
> On Linux MAP_FAILED is not the same as NULL.
> 
> Since lots of other calls check for MAP_FAILED, fix to use
> MAP_FAILED.
> 
> tools/blktap/drivers/tapdisk.c does check for -1 not MAP_FAILED.  I
> did not change since it was not clearly wrong.

I've acked + applied #2..#4. I replied to #1 which I think is wrong.

Ian.