Skip Menu |

This queue is for tickets about the Text-CSV_XS CPAN distribution.

Report information
The Basics
Id: 81469
Status: resolved
Priority: 0/
Queue: Text-CSV_XS
People
Owner: Nobody in particular
Requestors: RURBAN [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: Important
Broken in: 0.69
Fixed in: 0.94

History Show all quoted text
  Mon Nov 26 17:16:32 2012 RURBAN [...] cpan.org - Ticket created
Subject: Fix sv_cache init global-buffer-overflow
sv_cache = newSVpvn ("", CACHE_SIZE); must be replaced with sv_cache = newSVpvn ("\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0 \0\0\0\0\0", CACHE_SIZE); 40x "\0" sv_setpvn is not safe to use when the length is longer then string, the Move(ptr,dptr,len,char) overwrites memory belonging to other variables. Only asan detected it. See attached patch. -- Reini Urban
Subject: Text-CSV_XS-0.93.patch
diff -bu ./CSV_XS.xs~ ./CSV_XS.xs --- ./CSV_XS.xs~ 2012-11-19 09:18:05.000000000 -0600 +++ ./CSV_XS.xs 2012-11-26 16:05:43.410586807 -0600 @@ -522,9 +522,8 @@ csv->verbatim = bool_opt ("verbatim"); csv->auto_diag = bool_opt ("auto_diag"); - sv_cache = newSVpvn ("", CACHE_SIZE); + sv_cache = newSVpvn ("\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", CACHE_SIZE); csv->cache = (byte *)SvPVX (sv_cache); - memset (csv->cache, 0, CACHE_SIZE); SvREADONLY_on (sv_cache); csv->cache[CACHE_ID_quote_char] = csv->quote_char;
  Tue Nov 27 04:14:02 2012 h.m.brand [...] xs4all.nl - Correspondence added
Subject: Re: [rt.cpan.org #81469] Fix sv_cache init global-buffer-overflow
Date: Tue, 27 Nov 2012 10:13:43 +0100
To: bug-Text-CSV_XS [...] rt.cpan.org
From: "H.Merijn Brand" <h.m.brand [...] xs4all.nl>
On Mon, 26 Nov 2012 17:16:33 -0500, "Reini Urban via RT" <bug-Text-CSV_XS@rt.cpan.org> wrote: Show quoted text
> sv_cache = newSVpvn ("", CACHE_SIZE); > > must be replaced with > > sv_cache = newSVpvn ("\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", CACHE_SIZE); > > 40x "\0" > > sv_setpvn is not safe to use when the length is longer then string, the > Move (ptr, dptr, len, char) overwrites memory belonging to other variables. > > Only asan detected it. See attached patch.
Would this be equally safe, as it is easier to maintain and clearer code: diff --git a/CSV_XS.xs b/CSV_XS.xs index 3ac8d60..4f88b77 100644 --- a/CSV_XS.xs +++ b/CSV_XS.xs @@ -194,6 +194,7 @@ xs_error_t xs_errors[] = { { 0, "" }, }; +static byte init_cache[CACHE_SIZE]; static int io_handle_loaded = 0; static SV *m_getline, *m_print, *m_read; @@ -522,9 +523,8 @@ static void cx_SetupCsv (pTHX_ csv_t *csv, HV *self, SV *pself) csv->verbatim = bool_opt ("verbatim"); csv->auto_diag = bool_opt ("auto_diag"); - sv_cache = newSVpvn ("", CACHE_SIZE); + sv_cache = newSVpvn (init_cache, CACHE_SIZE); csv->cache = (byte *)SvPVX (sv_cache); - memset (csv->cache, 0, CACHE_SIZE); SvREADONLY_on (sv_cache); csv->cache[CACHE_ID_quote_char] = csv->quote_char; -- H.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/ using perl5.00307 .. 5.17 porting perl5 on HP-UX, AIX, and openSUSE http://mirrors.develooper.com/hpux/ http://www.test-smoke.org/ http://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/
  Tue Nov 27 04:14:03 2012 The RT System itself - Status changed from 'new' to 'open'
  Tue Nov 27 13:41:29 2012 rurban [...] x-ray.at - Correspondence added
Subject: Re: [rt.cpan.org #81469] Fix sv_cache init global-buffer-overflow
Date: Tue, 27 Nov 2012 12:41:17 -0600
To: bug-Text-CSV_XS [...] rt.cpan.org
From: Reini Urban <rurban [...] x-ray.at>
On Tue, Nov 27, 2012 at 3:14 AM, h.m.brand@xs4all.nl via RT <bug-Text-CSV_XS@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=81469 > > > On Mon, 26 Nov 2012 17:16:33 -0500, "Reini Urban via RT" > <bug-Text-CSV_XS@rt.cpan.org> wrote: >
>> sv_cache = newSVpvn ("", CACHE_SIZE); >> >> must be replaced with >> >> sv_cache = newSVpvn ("\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", CACHE_SIZE); >> >> 40x "\0" >> >> sv_setpvn is not safe to use when the length is longer then string, the >> Move (ptr, dptr, len, char) overwrites memory belonging to other variables. >> >> Only asan detected it. See attached patch.
> > Would this be equally safe, as it is easier to maintain and clearer > code: > > diff --git a/CSV_XS.xs b/CSV_XS.xs > index 3ac8d60..4f88b77 100644 > --- a/CSV_XS.xs > +++ b/CSV_XS.xs > @@ -194,6 +194,7 @@ xs_error_t xs_errors[] = { > { 0, "" }, > }; > > +static byte init_cache[CACHE_SIZE]; > static int io_handle_loaded = 0; > static SV *m_getline, *m_print, *m_read; > > @@ -522,9 +523,8 @@ static void cx_SetupCsv (pTHX_ csv_t *csv, HV *self, SV *pself) > csv->verbatim = bool_opt ("verbatim"); > csv->auto_diag = bool_opt ("auto_diag"); > > - sv_cache = newSVpvn ("", CACHE_SIZE); > + sv_cache = newSVpvn (init_cache, CACHE_SIZE); > csv->cache = (byte *)SvPVX (sv_cache); > - memset (csv->cache, 0, CACHE_SIZE); > SvREADONLY_on (sv_cache); > > csv->cache[CACHE_ID_quote_char] = csv->quote_char;
Indeed, your version looks better and works ok. But then you have to keep the memset (csv->cache, 0, CACHE_SIZE); -- Reini Urban http://cpanel.net/ http://www.perl-compiler.org/
  Wed Nov 28 02:24:25 2012 h.m.brand [...] xs4all.nl - Correspondence added
Subject: Re: [rt.cpan.org #81469] Fix sv_cache init global-buffer-overflow
Date: Wed, 28 Nov 2012 08:24:12 +0100
To: bug-Text-CSV_XS [...] rt.cpan.org
From: "H.Merijn Brand" <h.m.brand [...] xs4all.nl>
On Tue, 27 Nov 2012 13:41:30 -0500, "Reini Urban via RT" <bug-Text-CSV_XS@rt.cpan.org> wrote: Show quoted text
> > Would this be equally safe, as it is easier to maintain and clearer > > code: > > > > diff --git a/CSV_XS.xs b/CSV_XS.xs > > index 3ac8d60..4f88b77 100644 > > --- a/CSV_XS.xs > > +++ b/CSV_XS.xs > > @@ -194,6 +194,7 @@ xs_error_t xs_errors[] = { > > { 0, "" }, > > }; > > > > +static byte init_cache[CACHE_SIZE]; > > static int io_handle_loaded = 0; > > static SV *m_getline, *m_print, *m_read; > > > > @@ -522,9 +523,8 @@ static void cx_SetupCsv (pTHX_ csv_t *csv, HV *self, SV *pself) > > csv->verbatim = bool_opt ("verbatim"); > > csv->auto_diag = bool_opt ("auto_diag"); > > > > - sv_cache = newSVpvn ("", CACHE_SIZE); > > + sv_cache = newSVpvn (init_cache, CACHE_SIZE); > > csv->cache = (byte *)SvPVX (sv_cache); > > - memset (csv->cache, 0, CACHE_SIZE); > > SvREADONLY_on (sv_cache); > > > > csv->cache[CACHE_ID_quote_char] = csv->quote_char;
> > Indeed, your version looks better and works ok. > But then you have to keep the > memset (csv->cache, 0, CACHE_SIZE);
Why? C-standard guarantees that static declared variables are initialized with \0. Or ar you telling me that the source is *used* instead of *copied*. http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf 6.2.4 3. … An object whose identifier is declared with external or internal linkage, or with the storage-class specifier static has static storage duration. Its lifetime is the entire execution of the program and its stored value is initialized only once, prior to program startup. 6.7.8 10. … If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static storage duration is not initialized explicitly, then: - if it has pointer type, it is initialized to a null pointer; - if it has arithmetic type, it is initialized to (positive or unsigned) zero; - if it is an aggregate, every member is initialized (recursively) according to these rules; - if it is a union, the first named member is initialized (recursively) according to these rules. -- H.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/ using perl5.00307 .. 5.17 porting perl5 on HP-UX, AIX, and openSUSE http://mirrors.develooper.com/hpux/ http://www.test-smoke.org/ http://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/
  Wed Nov 28 02:46:17 2012 HMBRAND [...] cpan.org - Broken in 0.69 added
  Wed Nov 28 02:46:17 2012 HMBRAND [...] cpan.org - Broken in 0.93 deleted
  Wed Nov 28 02:46:17 2012 HMBRAND [...] cpan.org - Status changed from 'open' to 'patched'
  Wed Nov 28 11:48:42 2012 rurban [...] x-ray.at - Correspondence added
Subject: Re: [rt.cpan.org #81469] Fix sv_cache init global-buffer-overflow
Date: Wed, 28 Nov 2012 10:48:30 -0600
To: bug-Text-CSV_XS [...] rt.cpan.org
From: Reini Urban <rurban [...] x-ray.at>
On Wed, Nov 28, 2012 at 1:24 AM, h.m.brand@xs4all.nl via RT <bug-Text-CSV_XS@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=81469 > > On Tue, 27 Nov 2012 13:41:30 -0500, "Reini Urban via RT" > <bug-Text-CSV_XS@rt.cpan.org> wrote: >
>> > Would this be equally safe, as it is easier to maintain and clearer >> > code: >> > >> > diff --git a/CSV_XS.xs b/CSV_XS.xs >> > index 3ac8d60..4f88b77 100644 >> > --- a/CSV_XS.xs >> > +++ b/CSV_XS.xs >> > @@ -194,6 +194,7 @@ xs_error_t xs_errors[] = { >> > { 0, "" }, >> > }; >> > >> > +static byte init_cache[CACHE_SIZE]; >> > static int io_handle_loaded = 0; >> > static SV *m_getline, *m_print, *m_read; >> > >> > @@ -522,9 +523,8 @@ static void cx_SetupCsv (pTHX_ csv_t *csv, HV *self, SV *pself) >> > csv->verbatim = bool_opt ("verbatim"); >> > csv->auto_diag = bool_opt ("auto_diag"); >> > >> > - sv_cache = newSVpvn ("", CACHE_SIZE); >> > + sv_cache = newSVpvn (init_cache, CACHE_SIZE); >> > csv->cache = (byte *)SvPVX (sv_cache); >> > - memset (csv->cache, 0, CACHE_SIZE); >> > SvREADONLY_on (sv_cache); >> > >> > csv->cache[CACHE_ID_quote_char] = csv->quote_char;
>> >> Indeed, your version looks better and works ok. >> But then you have to keep the >> memset (csv->cache, 0, CACHE_SIZE);
> > Why? C-standard guarantees that static declared variables are > initialized with \0. Or ar you telling me that the source is > *used* instead of *copied*.
Oh, I did not know that. Nice, thanks. Show quoted text
> http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf > > > 6.2.4 3. … > > An object whose identifier is declared with external or internal > linkage, or with the storage-class specifier static has static > storage duration. Its lifetime is the entire execution of the > program and its stored value is initialized only once, prior to > program startup. > > 6.7.8 10. … > > If an object that has automatic storage duration is not initialized > explicitly, its value is indeterminate. If an object that has static > storage duration is not initialized explicitly, then: > > - if it has pointer type, it is initialized to a null pointer; > - if it has arithmetic type, it is initialized to (positive or > unsigned) zero; > - if it is an aggregate, every member is initialized (recursively) > according to these rules; > - if it is a union, the first named member is initialized > (recursively) according to these rules.
-- Reini Urban http://cpanel.net/ http://www.perl-compiler.org/
  Wed Nov 28 11:48:43 2012 The RT System itself - Status changed from 'patched' to 'open'
  Mon Dec 03 17:39:06 2012 HMBRAND [...] cpan.org - Fixed in 0.94 added
  Mon Dec 03 17:39:06 2012 HMBRAND [...] cpan.org - Status changed from 'open' to 'resolved'