Thread: [Plib-devel] [PATCH] ssg/ssgLoadAC.cxx: fix potential crash; accept DOS line endings
Brought to you by:
sjbaker
Menu
▾
▴
plib-devel
|
From: Melchior F. <a86...@un...> - 2003-07-02 12:39:45
|
1. add a missing list stopper to avoid potential crashes
2. most of ssgLoadAC.cxx is able to deal with AC files with
DOS line endings, only one line breaks it and leads to
"ac_to_gl: Unrecognised token ''".
do_data():360 discards the expected '\n', which may
actually be a '\r' with the real '\n' coming next and
screwing things up.
Please consider to apply. (And please do also apply my sound
patch from 2003/6/19.)
m.
Index: ssgLoadAC.cxx
===================================================================
RCS file: /cvsroot/plib/plib/src/ssg/ssgLoadAC.cxx,v
retrieving revision 1.30
diff -u -p -r1.30 ssgLoadAC.cxx
--- ssgLoadAC.cxx 15 Sep 2002 01:29:11 -0000 1.30
+++ ssgLoadAC.cxx 2 Jul 2003 12:36:25 -0000
@@ -138,6 +138,7 @@ static Tag top_tags [] =
{
{ "MATERIAL", do_material },
{ "OBJECT" , do_object },
+ { NULL, NULL }
} ;
@@ -356,7 +357,12 @@ static int do_data ( char *s )
current_data [ len ] = '\0' ;
- getc ( loader_fd ) ; /* Final RETURN */
+ int c;
+ while ( ( c = getc( loader_fd ) ) != EOF ) /* Final RETURN */
+ if ( c != '\r' && c != '\n' ) {
+ ungetc ( c, loader_fd ) ;
+ break ;
+ }
ssgBranch *br = current_options -> createBranch ( current_data ) ;
|
|
From: Steve B. <sjb...@ai...> - 2003-07-03 03:34:18
|
Melchior FRANZ wrote: > 1. add a missing list stopper to avoid potential crashes > > 2. most of ssgLoadAC.cxx is able to deal with AC files with > DOS line endings, only one line breaks it and leads to > "ac_to_gl: Unrecognised token ''". > do_data():360 discards the expected '\n', which may > actually be a '\r' with the real '\n' coming next and > screwing things up. '.ac' files don't have DOS line endings - even the ones written by the Windoze version of AC3D. However, the patch isn't unreasonable - go for it. ---------------------------- Steve Baker ------------------------- HomeEmail: <sjb...@ai...> WorkEmail: <sj...@li...> HomePage : http://www.sjbaker.org Projects : http://plib.sf.net http://tuxaqfh.sf.net http://tuxkart.sf.net http://prettypoly.sf.net -----BEGIN GEEK CODE BLOCK----- GCS d-- s:+ a+ C++++$ UL+++$ P--- L++++$ E--- W+++ N o+ K? w--- !O M- V-- PS++ PE- Y-- PGP-- t+ 5 X R+++ tv b++ DI++ D G+ e++ h--(-) r+++ y++++ -----END GEEK CODE BLOCK----- |
|
From: Melchior F. <a86...@un...> - 2003-07-03 05:27:39
|
* Steve Baker -- Thursday 03 July 2003 05:36: > '.ac' files don't have DOS line endings - even the ones written > by the Windoze version of AC3D. I suspected that already, but there had something to be done. Either a useful error report (not just an unrecognized empty token), or allowing the nonsensical DOS line ends. As both solutions would have taken about the same lines of code, I went for the more tolerant one. :-) > However, the patch isn't unreasonable - go for it. Err ... could someone apply then, please? m. |
|
From: Frederic B. <fr...@wa...> - 2003-07-03 06:51:55
|
Melchior FRANZ writes: > * Steve Baker -- Thursday 03 July 2003 05:36: > > '.ac' files don't have DOS line endings - even the ones written > > by the Windoze version of AC3D. > > I suspected that already, but there had something to be done. > Either a useful error report (not just an unrecognized empty > token), or allowing the nonsensical DOS line ends. As both > solutions would have taken about the same lines of code, I went > for the more tolerant one. :-) There is no problem either under Linux or Windows using MSVC. In the first case, there is no CR, in the second, the CR is consumed by the file routines when the file is open in text mode. The problem arise when the file is read by cygwin that is configured with unix line endings, opening the file in binary mode. There are CR in the file because it is fetched with CVS on windows. -Fred |
|
From: Melchior F. <a86...@un...> - 2003-07-03 07:07:44
|
* Frederic Bouvier -- Thursday 03 July 2003 08:51: > There is no problem either under Linux or Windows using MSVC. > In the first case, there is no CR, in the second, the CR is > consumed by the file routines when the file is open in text mode. > > The problem arise when the file is read by cygwin that is > configured with unix line endings, opening the file in binary > mode. So what? Users are confronted with the error message "ac_to_gl: Unrecognised token ''". That's not made up, but reality. And, despite violating the ac spec, these files happen to contain \r\n, be it only on cygwin systems. And that's no surprise, given that $ file a320-fb.ac a320-fb.ac: ASCII text What' wrong with being tolerant? It may be desirable, though, to output a warning. m. |
|
From: Bernie B. <bb...@bi...> - 2003-07-03 09:04:56
|
On Wed, 02 Jul 2003 22:36:06 -0500
Steve Baker <sjb...@ai...> wrote:
> Melchior FRANZ wrote:
> > 1. add a missing list stopper to avoid potential crashes
> >
> > 2. most of ssgLoadAC.cxx is able to deal with AC files with
> > DOS line endings, only one line breaks it and leads to
> > "ac_to_gl: Unrecognised token ''".
> > do_data():360 discards the expected '\n', which may
> > actually be a '\r' with the real '\n' coming next and
> > screwing things up.
>
> '.ac' files don't have DOS line endings - even the ones written
> by the Windoze version of AC3D.
>
> However, the patch isn't unreasonable - go for it.
Instead of the specific patch to do_data() what if skip_space() actually
skipped all whitespace characters instead of just ' ' and '\t':
while (isspace(**s))
(*s)++;
I'm not familiar with the ac format and I haven't tested this but it seems a
better fix to a more general problem.
Bernie
|
|
From: Melchior F. <a86...@un...> - 2003-07-03 09:13:16
|
* Bernie Bright -- Thursday 03 July 2003 11:04: > Instead of the specific patch to do_data() what if skip_space() actually > skipped all whitespace characters instead of just ' ' and '\t': No. skip_space is applied to a string pointer, while in do_data we are directly reading from a stream. There would have to be a lot more to rewrite to make your suggestion work. m. |
|
From: Melchior F. <a86...@un...> - 2003-07-03 15:26:26
|
* Melchior FRANZ -- Thursday 03 July 2003 11:12:
> * Bernie Bright -- Thursday 03 July 2003 11:04:
> > Instead of the specific patch to do_data() what if skip_space() actually
> > skipped all whitespace characters instead of just ' ' and '\t':
>
> No. skip_space is applied to a string pointer, while in do_data
> we are directly reading from a stream. There would have to be
> a lot more to rewrite to make your suggestion work.
OK, I was exaggerating quite a bit. Yes, do_data does directly
read from the fd, but there's no need to do so. You won. Attached
is a better fix. It should behave exactly as the original function.
But it doesn't swallow an arbitrary number of trailing \n and \r,
but only one set, which should suffice.
m. :-)
diff -u -p -r1.30 ssgLoadAC.cxx
--- ssgLoadAC.cxx 15 Sep 2002 01:29:11 -0000 1.30
+++ ssgLoadAC.cxx 3 Jul 2003 15:22:35 -0000
@@ -138,6 +138,7 @@ static Tag top_tags [] =
{
{ "MATERIAL", do_material },
{ "OBJECT" , do_object },
+ { NULL, NULL }
} ;
@@ -345,18 +346,15 @@ static int do_name ( char *s )
}
-static int do_data ( char *s )
+static int do_data ( char *s )
{
int len = strtol ( s, NULL, 0 ) ;
+ char buffer [ len + 3 ] ; /* data + \r + \n + \0 */
+ fgets ( buffer, len + 3, loader_fd ) ;
current_data = new char [ len + 1 ] ;
-
- for ( int i = 0 ; i < len ; i++ )
- current_data [ i ] = getc ( loader_fd ) ;
-
- current_data [ len ] = '\0' ;
-
- getc ( loader_fd ) ; /* Final RETURN */
+ strncpy ( current_data, buffer, len ) ;
+ current_data [ len ] = '\0' ;
ssgBranch *br = current_options -> createBranch ( current_data ) ;
|
|
From: James J. <jcp...@co...> - 2003-07-04 13:53:10
|
On Thu, 2003-07-03 at 10:25, Melchior FRANZ wrote: > * Melchior FRANZ -- Thursday 03 July 2003 11:12: > > * Bernie Bright -- Thursday 03 July 2003 11:04: > > > Instead of the specific patch to do_data() what if skip_space() actually > > > skipped all whitespace characters instead of just ' ' and '\t': > > > > No. skip_space is applied to a string pointer, while in do_data > > we are directly reading from a stream. There would have to be > > a lot more to rewrite to make your suggestion work. > > OK, I was exaggerating quite a bit. Yes, do_data does directly > read from the fd, but there's no need to do so. You won. Attached > is a better fix. It should behave exactly as the original function. > But it doesn't swallow an arbitrary number of trailing \n and \r, > but only one set, which should suffice. > This patch and the two from Martin Spott for Solaris have been applied to CVS. Thanks, folks! -- James 'J.C.' Jones We are Pentium of Borg. Division is futile. You will be approximated. |
|
From: Erik H. <er...@eh...> - 2003-07-04 14:40:35
|
James Jones wrote: > This patch and the two from Martin Spott for Solaris have been applied > to CVS. Could you add this patch also: http://sourceforge.net/mailarchive/forum.php?thread_id=2617842&forum_id=4479 Erik |
|
From: Martin S. <Mar...@un...> - 2003-07-05 15:23:15
|
James Jones <jcp...@co...> wrote: > This patch and the two from Martin Spott for Solaris have been applied > to CVS. Very nice. PLIB now compiles out of the box on Solaris8/Sparc. Thanks, Martin. -- Unix _IS_ user friendly - it's just selective about who its friends are ! -------------------------------------------------------------------------- |
|
From: Frederic B. <fr...@wa...> - 2003-07-04 21:00:40
|
Melchior FRANZ writes:
> * Melchior FRANZ -- Thursday 03 July 2003 11:12:
> > * Bernie Bright -- Thursday 03 July 2003 11:04:
> > > Instead of the specific patch to do_data() what if skip_space()
actually
> > > skipped all whitespace characters instead of just ' ' and '\t':
> >
> > No. skip_space is applied to a string pointer, while in do_data
> > we are directly reading from a stream. There would have to be
> > a lot more to rewrite to make your suggestion work.
>
> OK, I was exaggerating quite a bit. Yes, do_data does directly
> read from the fd, but there's no need to do so. You won. Attached
> is a better fix. It should behave exactly as the original function.
> But it doesn't swallow an arbitrary number of trailing \n and \r,
> but only one set, which should suffice.
>
> m. :-)
>
>
>
> diff -u -p -r1.30 ssgLoadAC.cxx
> --- ssgLoadAC.cxx 15 Sep 2002 01:29:11 -0000 1.30
> +++ ssgLoadAC.cxx 3 Jul 2003 15:22:35 -0000
> @@ -138,6 +138,7 @@ static Tag top_tags [] =
> {
> { "MATERIAL", do_material },
> { "OBJECT" , do_object },
> + { NULL, NULL }
> } ;
>
>
> @@ -345,18 +346,15 @@ static int do_name ( char *s )
> }
>
>
> -static int do_data ( char *s )
> +static int do_data ( char *s )
> {
> int len = strtol ( s, NULL, 0 ) ;
> + char buffer [ len + 3 ] ; /* data + \r + \n + \0 */
> + fgets ( buffer, len + 3, loader_fd ) ;
>
> current_data = new char [ len + 1 ] ;
> -
> - for ( int i = 0 ; i < len ; i++ )
> - current_data [ i ] = getc ( loader_fd ) ;
> -
> - current_data [ len ] = '\0' ;
> -
> - getc ( loader_fd ) ; /* Final RETURN */
> + strncpy ( current_data, buffer, len ) ;
> + current_data [ len ] = '\0' ;
>
> ssgBranch *br = current_options -> createBranch ( current_data ) ;
>
This patch produces this on MSVC :
Compiling...
ssgLoadAC.cxx
ssgLoadAC.cxx(352) : error C2057: expected constant expression
ssgLoadAC.cxx(352) : error C2466: cannot allocate an array of constant size
0
ssgLoadAC.cxx(352) : error C2133: 'buffer' : unknown size
line 352 is :
char buffer [ len + 3 ] ; /* data + \r + \n + \0 */
It seems to not allow allocation of a static array with a variable size.
-Fred
|
|
From: Frederic B. <fr...@wa...> - 2003-07-04 21:07:42
|
Frederic Bouvier writes:
>
> This patch produces this on MSVC :
>
> Compiling...
> ssgLoadAC.cxx
> ssgLoadAC.cxx(352) : error C2057: expected constant expression
> ssgLoadAC.cxx(352) : error C2466: cannot allocate an array of constant
size
> 0
> ssgLoadAC.cxx(352) : error C2133: 'buffer' : unknown size
>
> line 352 is :
> char buffer [ len + 3 ] ; /* data + \r + \n + \0 */
>
>
> It seems to not allow allocation of a static array with a variable size.
I propose the patch below. It avoids a strcpy at the expense of two bytes :
-Fred
RCS file: /cvsroot/plib/plib/src/ssg/ssgLoadAC.cxx,v
retrieving revision 1.31
diff -u -r1.31 ssgLoadAC.cxx
--- ssgLoadAC.cxx 4 Jul 2003 13:51:58 -0000 1.31
+++ ssgLoadAC.cxx 4 Jul 2003 21:03:58 -0000
@@ -349,12 +349,10 @@
static int do_data ( char *s )
{
int len = strtol ( s, NULL, 0 ) ;
- char buffer [ len + 3 ] ; /* data + \r + \n + \0 */
- fgets ( buffer, len + 3, loader_fd ) ;
- current_data = new char [ len + 1 ] ;
+ current_data = new char [ len + 3 ] ;
+ fgets ( current_data, len + 3, loader_fd ) ;
- strncpy ( current_data, buffer, len ) ;
current_data [ len ] = '\0' ;
ssgBranch *br = current_options -> createBranch ( current_data ) ;
|
|
From: Melchior F. <a86...@un...> - 2003-07-05 07:27:02
|
* Frederic Bouvier -- Friday 04 July 2003 23:07: > This patch produces this on MSVC : [...] > ssgLoadAC.cxx(352) : error C2466: cannot allocate an array of constant [...] > I propose the patch below. It avoids a strcpy at the expense of two bytes : That's what I had before, but I dismissed it because of the wasted two bytes. I didn't know how much memory this would make in a whole ssg tree. Apart from that it's of course better (and faster). If the two bytes are a problem, you can still revert and apply the patch that I had sent first (-> start of the thread). m. |
|
From: Frederic B. <fr...@wa...> - 2003-07-05 07:36:05
|
Melchior FRANZ writes: > * Frederic Bouvier -- Friday 04 July 2003 23:07: > > This patch produces this on MSVC : > [...] > > ssgLoadAC.cxx(352) : error C2466: cannot allocate an array of constant > [...] > > I propose the patch below. It avoids a strcpy at the expense of two bytes : > > That's what I had before, but I dismissed it because of the wasted > two bytes. I didn't know how much memory this would make in a whole > ssg tree. Apart from that it's of course better (and faster). > If the two bytes are a problem, you can still revert and apply the > patch that I had sent first (-> start of the thread). I think memory allocation is made by chunks of at least 16 bytes, so these two bytes wouldn't make a difference on most allocation. -Fred |
|
From: Paul B. <pb...@ax...> - 2003-07-05 19:40:43
|
On my system ssgLoadAC.cxx is broken since pulling the latest CVS. It works in my own test programs, but Tuxkart 0.2.0/CVS, and tux_aqfh-1.0.14 fail with the following error: FATAL: ac_to_gl: Unrecognised token ' ' I have not yet figured out the exact cause. I'm compiling under Linux (Mardrake 8.1): gcc version 2.96 20000731 (Mandrake Linux 8.1 2.96-0.62mdk) Mesa 3.4.2 On Wednesday 02 July 2003 10:36 pm, Steve Baker wrote: > Melchior FRANZ wrote: > > 1. add a missing list stopper to avoid potential crashes > > > > 2. most of ssgLoadAC.cxx is able to deal with AC files with > > DOS line endings, only one line breaks it and leads to > > "ac_to_gl: Unrecognised token ''". > > do_data():360 discards the expected '\n', which may > > actually be a '\r' with the real '\n' coming next and > > screwing things up. > > '.ac' files don't have DOS line endings - even the ones written > by the Windoze version of AC3D. > > However, the patch isn't unreasonable - go for it. > ..snip... -- Paul Boese pb...@ax... |
|
From: Melchior F. <a86...@un...> - 2003-07-06 13:37:49
|
* Paul Boese -- Saturday 05 July 2003 21:40: > On my system ssgLoadAC.cxx is broken since pulling the latest CVS. It works > in my own test programs, but Tuxkart 0.2.0/CVS, and tux_aqfh-1.0.14 fail with > the following error: > > FATAL: ac_to_gl: Unrecognised token ' > ' > > I have not yet figured out the exact cause. The exact cause is lots of strange ac files, containing things like ==== tuxaqfh/models/folly.ac data 14 @autodcs h=22 texture "../images/swirl.rgb" ==== That is, the data field contains an \n. Maybe this was the cause for how the original code was written like? In this case I'd revert the last patch and apply the one that I've presented first in this very thread. m. |
|
From: Paul B. <pb...@ax...> - 2003-07-06 16:47:43
|
I had a feeling about that. I did not take the time to test it out using multiple lines in the data field. That makes sense now. On Sunday 06 July 2003 08:37 am, Melchior FRANZ wrote: > The exact cause is lots of strange ac files, containing things like > > ==== tuxaqfh/models/folly.ac > data 14 > @autodcs h=22 > > texture "../images/swirl.rgb" > ==== > > > That is, the data field contains an \n. Maybe this was the cause for > how the original code was written like? In this case I'd revert the > last patch and apply the one that I've presented first in this very > thread. > > m. -- Paul Boese pb...@ax... |
|
From: Paul B. <pb...@ax...> - 2003-07-06 17:37:19
|
Okay I reverted and applied your original patch as suggested. It does the job and works as advertised on Linux. Sorry I don't run Cygwin here so I can't test in that environment. Pb On Sunday 06 July 2003 08:37 am, Melchior FRANZ wrote: > * Paul Boese -- Saturday 05 July 2003 21:40: > > On my system ssgLoadAC.cxx is broken since pulling the latest CVS. It > > works in my own test programs, but Tuxkart 0.2.0/CVS, and tux_aqfh-1.0.14 > > fail with the following error: > > > > FATAL: ac_to_gl: Unrecognised token ' > > ' > > > > I have not yet figured out the exact cause. > > The exact cause is lots of strange ac files, containing things like > > ==== tuxaqfh/models/folly.ac > data 14 > @autodcs h=22 > > texture "../images/swirl.rgb" > ==== > > > That is, the data field contains an \n. Maybe this was the cause for > how the original code was written like? In this case I'd revert the > last patch and apply the one that I've presented first in this very > thread. > > m. ...snip... -- Paul Boese pb...@ax... |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X