Re: kernel 2.4.27-10: isofs driver ignore some parameters with mount

[Horms <horms@debian.org>]

On Mon, Aug 15, 2005 at 10:11:21PM -0300, Marcelo Tosatti wrote:
> 
> Hi folks,
> 
> On Fri, Aug 12, 2005 at 05:29:36PM +0900, Horms wrote:
> > On Fri, Aug 12, 2005 at 10:44:17AM +0300, Alexander Pytlev wrote:
> > > Hello Debian,
> > > 
> > > Kernel 2.4.27-10
> > > With mount isofs filesystem, any mount parameters after
> > > iocharset=,map=,session= are ignored.
> > > 
> > > Sample:
> > > 
> > > mount -t isofs -o uid=100,iocharset=koi8-r,gid=100 /dev/cdrom /media/cdrom
> > > 
> > > gid=100 - was ignored
> > > 
> > > I look in source and find that problem. I make two patch, simply and full
> > > (what addeded some functionality - ignore wrong mount parameters)
> > 
> > Thanks,
> > 
> > I will try and get the simple version of this patch into the next
> > Sarge update.
> > 
> > I have also CCed Marcelo and the LKML for their consideration,
> > as this problem still seems to be present in the lastest 2.4 tree.
> > 
> > -- 
> > Horms
> > 
> > simply patch:
> > ===================================================================================
> > --- kernel-source-2.4.27/fs/isofs/inode.c       2005-05-19 13:29:39.000000000 +0300
> > +++ kernel-source/fs/isofs/inode.c      2005-08-11 11:55:12.000000000 +0300
> > @@ -340,13 +340,13 @@
> >                         else if (!strcmp(value,"acorn")) popt->map = 'a';
> >                         else return 0;
> >                 }
> > -               if (!strcmp(this_char,"session") && value) {
> > +               else if (!strcmp(this_char,"session") && value) {
> >                         char * vpnt = value;
> >                         unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
> >                         if(ivalue < 0 || ivalue >99) return 0;
> >                         popt->session=ivalue+1;
> >                 }
> > -               if (!strcmp(this_char,"sbsector") && value) {
> > +               else if (!strcmp(this_char,"sbsector") && value) {
> >                         char * vpnt = value;
> >                         unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
> >                         if(ivalue < 0 || ivalue >660*512) return 0;
> > ===================================================================================
> 
> Neither "sbsector" or "session" parameters are part of the options string used 
> in Alexander's example, so how come this patch can make any difference? 
> 
> Usage of "sbsector" or "session" parameters could explain the above patch
> making a difference because the buggy, always true "(unsigned long) ivalue < 0"
> comparison invokes "return 0", but that is not the case.
> 
> The code after the "popt->iocharset = value;" does not make any sense.
> 
> It seems that the "*value = 0" assignment can screw up the rest of the
> string, isnt that the real issue?
> 
> #ifdef CONFIG_JOLIET
>                 if (!strcmp(this_char,"iocharset") && value) {
>                         popt->iocharset = value;
>                         while (*value && *value != ',')
>                                 value++;
>                         if (value == popt->iocharset)
>                                 return 0;
>                         *value = 0;
>                 } else
> #endif

Sorry about that, while the patch above does seem to be
a valid clean up, on further examination I agree that it
does not address the problem at hand, and that the problem seems
to lie in the *value assignment as you suggest. I wonder
if advancing this_char to the character aftter value, if
non-NULL would resolve this problem. I'll do some testing,
but in the mean time, here is what I have in mind:

--- a/fs/isofs/inode.c	2005-08-16 14:22:27.000000000 +0900
+++ b/fs/isofs/inode.c	2005-08-16 14:27:55.000000000 +0900
@@ -329,7 +329,10 @@
 				value++;
 			if (value == popt->iocharset)
 				return 0;
-			*value = 0;
+			if (*value) {
+				this_char = value + 1;
+				*value = 0;
+			}
 		} else
 #endif
 		if (!strcmp(this_char,"map") && value) {