Syslinux - Apr 2013 - [syslinux:rockridge] iso9660.c did not copy terminating 0 of Rock Ridge name

On 04/25/2013 07:03 AM, syslinux-bot for Thomas Schmitt
wrote:
> Commit-ID:  5de463f724da515fd6c5ea49ded6dde178362181
> Gitweb:    
http://www.syslinux.org/commit/5de463f724da515fd6c5ea49ded6dde178362181
> Author:     Thomas Schmitt <scdbackup at gmx.net>
> AuthorDate: Thu, 4 Apr 2013 20:02:37 +0200
> Committer:  Matt Fleming <matt.fleming at intel.com>
> CommitDate: Thu, 25 Apr 2013 14:59:08 +0100
> 
> iso9660.c did not copy terminating 0 of Rock Ridge name
> 
> I noticed that the trailing 0-byte is not copied to the result of
> iso_readdir(). The function iso_convert_name() does append a trailing 0.
> 
> ---
>  core/fs/iso9660/iso9660.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Would it be possible for a (broken) disc to have a non-null-terminated
name here?

I'm wondering if it would be better to instead do:
> diff --git a/core/fs/iso9660/iso9660.c b/core/fs/iso9660/iso9660.c
> index 0f7b3d2..492adc6 100644
> --- a/core/fs/iso9660/iso9660.c
> +++ b/core/fs/iso9660/iso9660.c
> @@ -240,7 +240,7 @@ static int iso_readdir(struct file *file, struct dirent
*dirent)
>      /* Try to get Rock Ridge name */
>      ret = susp_rr_get_nm(fs, (char *) de, &rr_name, &name_len);
>      if (ret > 0) {
> -	memcpy(dirent->d_name, rr_name, name_len);
> +	memcpy(dirent->d_name, rr_name, name_len + 1);
memcpy(dirent->d_name, rr_name, name_len);
dirent->d_name[name_len] = '\0';

?

	-hpa

Hi,
> Would it be possible for a (broken) disc to have a non-null-terminated
> name here?
The descriptions of functions susp_rr_get_entries() and susp_rr_get_nm()
in susp_rr.h promise trailing 0-bytes.
This is implemented by line 376 in susp_rr.c:
        new_data[count] = 0;

My test call of susp_rr_get_nm() in libisofs/fs_image.c relies on this.
Test runs were done under control of valgrind.

> memcpy(dirent->d_name, rr_name, name_len);
> dirent->d_name[name_len] = '\0';
It cannot harm, at least.

I am still pondering how to realistically test the changes in iso9660.c.
(The code in susp_rr.[ch] is supposed to be quite well tested.)


BTW:
Matt's commit of my patch still does not show up in
  http://git.kernel.org/cgit/boot/syslinux/syslinux.git/log/?h=rockridge


Have a nice day :)

Thomas