1. Patchew
  2. Xen
  3. [PATCH v2 00/18] mini-os: remove struct file dependency on config
  4. View patch

[PATCH v2 16/18] mini-os: reset file type in close() in one place only

Juergen Gross posted 18 patches 4 years ago
There is a newer version of this series
[PATCH v2 16/18] mini-os: reset file type in close() in one place only
Posted by Juergen Gross 4 years ago 
Today the file type in struct file is set to FTYPE_NONE for each
file type individually. Do that at the end of close() handling for
all types.

While at it wipe the complete struct file, too, in order to avoid
old data creeping into a new allocated struct file.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 lib/sys.c | 53 ++++++++++++++++++++++++-----------------------------
 lib/xs.c  |  1 -
 2 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/lib/sys.c b/lib/sys.c
index 0e6fe5d..323a7cd 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -424,87 +424,82 @@ int fsync(int fd) {
 
 int close(int fd)
 {
+    int res = 0;
+
     printk("close(%d)\n", fd);
     switch (files[fd].type) {
         default:
-	    files[fd].type = FTYPE_NONE;
-	    return 0;
+            break;
 #ifdef CONFIG_XENBUS
 	case FTYPE_XENBUS:
             xs_daemon_close((void*)(intptr_t) fd);
-            return 0;
+            break;
 #endif
 #ifdef HAVE_LWIP
-	case FTYPE_SOCKET: {
-	    int res = lwip_close(files[fd].fd);
-	    files[fd].type = FTYPE_NONE;
-	    return res;
-	}
+	case FTYPE_SOCKET:
+	    res = lwip_close(files[fd].fd);
+            break;
 #endif
 #ifdef CONFIG_LIBXENCTRL
 	case FTYPE_XC:
 	    minios_interface_close_fd(fd);
-	    return 0;
+            break;
 #endif
 #ifdef CONFIG_LIBXENEVTCHN
 	case FTYPE_EVTCHN:
 	    minios_evtchn_close_fd(fd);
-            return 0;
+            break;
 #endif
 #ifdef CONFIG_LIBXENGNTTAB
 	case FTYPE_GNTMAP:
 	    minios_gnttab_close_fd(fd);
-	    return 0;
+            break;
 #endif
 #ifdef CONFIG_NETFRONT
 	case FTYPE_TAP:
 	    shutdown_netfront(files[fd].dev);
-	    files[fd].type = FTYPE_NONE;
-	    return 0;
+            break;
 #endif
 #ifdef CONFIG_BLKFRONT
 	case FTYPE_BLK:
             shutdown_blkfront(files[fd].dev);
-	    files[fd].type = FTYPE_NONE;
-	    return 0;
+            break;
 #endif
 #ifdef CONFIG_TPMFRONT
 	case FTYPE_TPMFRONT:
             shutdown_tpmfront(files[fd].dev);
-	    files[fd].type = FTYPE_NONE;
-	    return 0;
+            break;
 #endif
 #ifdef CONFIG_TPM_TIS
 	case FTYPE_TPM_TIS:
             shutdown_tpm_tis(files[fd].dev);
-	    files[fd].type = FTYPE_NONE;
-	    return 0;
+            break;
 #endif
 #ifdef CONFIG_KBDFRONT
 	case FTYPE_KBD:
             shutdown_kbdfront(files[fd].dev);
-            files[fd].type = FTYPE_NONE;
-            return 0;
+            break;
 #endif
 #ifdef CONFIG_FBFRONT
 	case FTYPE_FB:
             shutdown_fbfront(files[fd].dev);
-            files[fd].type = FTYPE_NONE;
-            return 0;
+            break;
 #endif
 #ifdef CONFIG_CONSFRONT
         case FTYPE_SAVEFILE:
         case FTYPE_CONSOLE:
             fini_consfront(files[fd].dev);
-            files[fd].type = FTYPE_NONE;
-            return 0;
+            break;
 #endif
 	case FTYPE_NONE:
-	    break;
+            printk("close(%d): Bad descriptor\n", fd);
+            errno = EBADF;
+            return -1;
     }
-    printk("close(%d): Bad descriptor\n", fd);
-    errno = EBADF;
-    return -1;
+
+    memset(files + fd, 0, sizeof(struct file));
+    files[fd].type = FTYPE_NONE;
+    return res;
 }
 
 static void init_stat(struct stat *buf)
diff --git a/lib/xs.c b/lib/xs.c
index 0459f52..4af0f96 100644
--- a/lib/xs.c
+++ b/lib/xs.c
@@ -35,7 +35,6 @@ void xs_daemon_close(struct xs_handle *h)
         next = event->next;
         free(event);
     }
-    files[fd].type = FTYPE_NONE;
 }
 
 int xs_fileno(struct xs_handle *h)
-- 
2.26.2
Re: [PATCH v2 16/18] mini-os: reset file type in close() in one place only
Posted by Samuel Thibault 4 years ago 
Juergen Gross, le mar. 11 janv. 2022 15:58:15 +0100, a ecrit:
> Today the file type in struct file is set to FTYPE_NONE for each
> file type individually. Do that at the end of close() handling for
> all types.
> 
> While at it wipe the complete struct file, too, in order to avoid
> old data creeping into a new allocated struct file.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Thanks!

> ---
> V2:
> - new patch
> ---
>  lib/sys.c | 53 ++++++++++++++++++++++++-----------------------------
>  lib/xs.c  |  1 -
>  2 files changed, 24 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/sys.c b/lib/sys.c
> index 0e6fe5d..323a7cd 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -424,87 +424,82 @@ int fsync(int fd) {
>  
>  int close(int fd)
>  {
> +    int res = 0;
> +
>      printk("close(%d)\n", fd);
>      switch (files[fd].type) {
>          default:
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #ifdef CONFIG_XENBUS
>  	case FTYPE_XENBUS:
>              xs_daemon_close((void*)(intptr_t) fd);
> -            return 0;
> +            break;
>  #endif
>  #ifdef HAVE_LWIP
> -	case FTYPE_SOCKET: {
> -	    int res = lwip_close(files[fd].fd);
> -	    files[fd].type = FTYPE_NONE;
> -	    return res;
> -	}
> +	case FTYPE_SOCKET:
> +	    res = lwip_close(files[fd].fd);
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENCTRL
>  	case FTYPE_XC:
>  	    minios_interface_close_fd(fd);
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENEVTCHN
>  	case FTYPE_EVTCHN:
>  	    minios_evtchn_close_fd(fd);
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENGNTTAB
>  	case FTYPE_GNTMAP:
>  	    minios_gnttab_close_fd(fd);
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_NETFRONT
>  	case FTYPE_TAP:
>  	    shutdown_netfront(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_BLKFRONT
>  	case FTYPE_BLK:
>              shutdown_blkfront(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_TPMFRONT
>  	case FTYPE_TPMFRONT:
>              shutdown_tpmfront(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_TPM_TIS
>  	case FTYPE_TPM_TIS:
>              shutdown_tpm_tis(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_KBDFRONT
>  	case FTYPE_KBD:
>              shutdown_kbdfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_FBFRONT
>  	case FTYPE_FB:
>              shutdown_fbfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_CONSFRONT
>          case FTYPE_SAVEFILE:
>          case FTYPE_CONSOLE:
>              fini_consfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  	case FTYPE_NONE:
> -	    break;
> +            printk("close(%d): Bad descriptor\n", fd);
> +            errno = EBADF;
> +            return -1;
>      }
> -    printk("close(%d): Bad descriptor\n", fd);
> -    errno = EBADF;
> -    return -1;
> +
> +    memset(files + fd, 0, sizeof(struct file));
> +    files[fd].type = FTYPE_NONE;
> +    return res;
>  }
>  
>  static void init_stat(struct stat *buf)
> diff --git a/lib/xs.c b/lib/xs.c
> index 0459f52..4af0f96 100644
> --- a/lib/xs.c
> +++ b/lib/xs.c
> @@ -35,7 +35,6 @@ void xs_daemon_close(struct xs_handle *h)
>          next = event->next;
>          free(event);
>      }
> -    files[fd].type = FTYPE_NONE;
>  }
>  
>  int xs_fileno(struct xs_handle *h)
> -- 
> 2.26.2
> 

-- 
Samuel
Pour un père, autant mourir que de faire plein de calculs et pas s'occuper
de son fils
 -+- y sur #ens-mim - sombres histoires de zombies -+-
Re: [PATCH v2 16/18] mini-os: reset file type in close() in one place only
Posted by Andrew Cooper 4 years ago 
On 11/01/2022 14:58, Juergen Gross wrote:
> diff --git a/lib/sys.c b/lib/sys.c
> index 0e6fe5d..323a7cd 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -424,87 +424,82 @@ int fsync(int fd) {
>  
>  int close(int fd)
>  {
> +    int res = 0;
> +
>      printk("close(%d)\n", fd);
>      switch (files[fd].type) {

I know this bug is pre-existing, but the libc close() really ought to
sanity check fd before blindly indexing files[] with it.

I'd tentatively suggest that you want one extra goto from here, into
wherever the EBADF logic ends up, and it's probably worth including in
this patch.

~Andrew
Re: [PATCH v2 16/18] mini-os: reset file type in close() in one place only
Posted by Juergen Gross 4 years ago 
On 11.01.22 20:14, Andrew Cooper wrote:
> On 11/01/2022 14:58, Juergen Gross wrote:
>> diff --git a/lib/sys.c b/lib/sys.c
>> index 0e6fe5d..323a7cd 100644
>> --- a/lib/sys.c
>> +++ b/lib/sys.c
>> @@ -424,87 +424,82 @@ int fsync(int fd) {
>>   
>>   int close(int fd)
>>   {
>> +    int res = 0;
>> +
>>       printk("close(%d)\n", fd);
>>       switch (files[fd].type) {
> 
> I know this bug is pre-existing, but the libc close() really ought to
> sanity check fd before blindly indexing files[] with it.
> 
> I'd tentatively suggest that you want one extra goto from here, into
> wherever the EBADF logic ends up, and it's probably worth including in
> this patch.

Will do that.


Juergen
Re: [PATCH v2 16/18] mini-os: reset file type in close() in one place only
Posted by Andrew Cooper 4 years ago 
On 11/01/2022 14:58, Juergen Gross wrote:
> diff --git a/lib/sys.c b/lib/sys.c
> index 0e6fe5d..323a7cd 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -424,87 +424,82 @@ int fsync(int fd) {
>  
>  int close(int fd)
>  {
> +    int res = 0;
> +
>      printk("close(%d)\n", fd);
>      switch (files[fd].type) {
>          default:
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #ifdef CONFIG_XENBUS
>  	case FTYPE_XENBUS:
>              xs_daemon_close((void*)(intptr_t) fd);
> -            return 0;
> +            break;
>  #endif
>  #ifdef HAVE_LWIP
> -	case FTYPE_SOCKET: {
> -	    int res = lwip_close(files[fd].fd);
> -	    files[fd].type = FTYPE_NONE;
> -	    return res;
> -	}
> +	case FTYPE_SOCKET:
> +	    res = lwip_close(files[fd].fd);

Hard tabs.

> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENCTRL
>  	case FTYPE_XC:
>  	    minios_interface_close_fd(fd);
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENEVTCHN
>  	case FTYPE_EVTCHN:
>  	    minios_evtchn_close_fd(fd);
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENGNTTAB
>  	case FTYPE_GNTMAP:
>  	    minios_gnttab_close_fd(fd);
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_NETFRONT
>  	case FTYPE_TAP:
>  	    shutdown_netfront(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_BLKFRONT
>  	case FTYPE_BLK:
>              shutdown_blkfront(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_TPMFRONT
>  	case FTYPE_TPMFRONT:
>              shutdown_tpmfront(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_TPM_TIS
>  	case FTYPE_TPM_TIS:
>              shutdown_tpm_tis(files[fd].dev);
> -	    files[fd].type = FTYPE_NONE;
> -	    return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_KBDFRONT
>  	case FTYPE_KBD:
>              shutdown_kbdfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_FBFRONT
>  	case FTYPE_FB:
>              shutdown_fbfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_CONSFRONT
>          case FTYPE_SAVEFILE:
>          case FTYPE_CONSOLE:
>              fini_consfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  	case FTYPE_NONE:
> -	    break;
> +            printk("close(%d): Bad descriptor\n", fd);
> +            errno = EBADF;
> +            return -1;
>      }
> -    printk("close(%d): Bad descriptor\n", fd);
> -    errno = EBADF;
> -    return -1;
> +
> +    memset(files + fd, 0, sizeof(struct file));
> +    files[fd].type = FTYPE_NONE;

BUILD_BUG_ON(FTYPE_NONE != 0);

Life's too short to deal with a theoretical (and short sighted) future
where someone might want to change FTYPE_NONE away from being 0.

~Andrew
Re: [PATCH v2 16/18] mini-os: reset file type in close() in one place only
Posted by Juergen Gross 4 years ago 
On 11.01.22 19:11, Andrew Cooper wrote:
> On 11/01/2022 14:58, Juergen Gross wrote:
>> diff --git a/lib/sys.c b/lib/sys.c
>> index 0e6fe5d..323a7cd 100644
>> --- a/lib/sys.c
>> +++ b/lib/sys.c
>> @@ -424,87 +424,82 @@ int fsync(int fd) {
>>   
>>   int close(int fd)
>>   {
>> +    int res = 0;
>> +
>>       printk("close(%d)\n", fd);
>>       switch (files[fd].type) {
>>           default:
>> -	    files[fd].type = FTYPE_NONE;
>> -	    return 0;
>> +            break;
>>   #ifdef CONFIG_XENBUS
>>   	case FTYPE_XENBUS:
>>               xs_daemon_close((void*)(intptr_t) fd);
>> -            return 0;
>> +            break;
>>   #endif
>>   #ifdef HAVE_LWIP
>> -	case FTYPE_SOCKET: {
>> -	    int res = lwip_close(files[fd].fd);
>> -	    files[fd].type = FTYPE_NONE;
>> -	    return res;
>> -	}
>> +	case FTYPE_SOCKET:
>> +	    res = lwip_close(files[fd].fd);
> 
> Hard tabs.
> 
>> +            break;
>>   #endif
>>   #ifdef CONFIG_LIBXENCTRL
>>   	case FTYPE_XC:
>>   	    minios_interface_close_fd(fd);
>> -	    return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_LIBXENEVTCHN
>>   	case FTYPE_EVTCHN:
>>   	    minios_evtchn_close_fd(fd);
>> -            return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_LIBXENGNTTAB
>>   	case FTYPE_GNTMAP:
>>   	    minios_gnttab_close_fd(fd);
>> -	    return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_NETFRONT
>>   	case FTYPE_TAP:
>>   	    shutdown_netfront(files[fd].dev);
>> -	    files[fd].type = FTYPE_NONE;
>> -	    return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_BLKFRONT
>>   	case FTYPE_BLK:
>>               shutdown_blkfront(files[fd].dev);
>> -	    files[fd].type = FTYPE_NONE;
>> -	    return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_TPMFRONT
>>   	case FTYPE_TPMFRONT:
>>               shutdown_tpmfront(files[fd].dev);
>> -	    files[fd].type = FTYPE_NONE;
>> -	    return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_TPM_TIS
>>   	case FTYPE_TPM_TIS:
>>               shutdown_tpm_tis(files[fd].dev);
>> -	    files[fd].type = FTYPE_NONE;
>> -	    return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_KBDFRONT
>>   	case FTYPE_KBD:
>>               shutdown_kbdfront(files[fd].dev);
>> -            files[fd].type = FTYPE_NONE;
>> -            return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_FBFRONT
>>   	case FTYPE_FB:
>>               shutdown_fbfront(files[fd].dev);
>> -            files[fd].type = FTYPE_NONE;
>> -            return 0;
>> +            break;
>>   #endif
>>   #ifdef CONFIG_CONSFRONT
>>           case FTYPE_SAVEFILE:
>>           case FTYPE_CONSOLE:
>>               fini_consfront(files[fd].dev);
>> -            files[fd].type = FTYPE_NONE;
>> -            return 0;
>> +            break;
>>   #endif
>>   	case FTYPE_NONE:
>> -	    break;
>> +            printk("close(%d): Bad descriptor\n", fd);
>> +            errno = EBADF;
>> +            return -1;
>>       }
>> -    printk("close(%d): Bad descriptor\n", fd);
>> -    errno = EBADF;
>> -    return -1;
>> +
>> +    memset(files + fd, 0, sizeof(struct file));
>> +    files[fd].type = FTYPE_NONE;
> 
> BUILD_BUG_ON(FTYPE_NONE != 0);
> 
> Life's too short to deal with a theoretical (and short sighted) future
> where someone might want to change FTYPE_NONE away from being 0.

Good idea.


Juergen

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.