From karthik.rajagopalan@schrodinger.com

Hi,

The following script access the dirTableA beyond the array limit and
might result in crash. We observed the issue at customer site where
PATH env was set with wrong delimiter in Windows and the script in
perl which they use to launch our jobs resulted in crash. Debugging
the perl code, showed the issue in vdir.h where dirTableA array was
being accessed beyond the limit. It is good to do bound check and make
perl not to crash. The attached patch fix the issue.

@​path = (

"C​:/Windows/system32",
".​:c​:/abc​:C​:/msys/1.0/scr/rajagopa/build05/Windows​:C​:/msys/1.0/scr/rajagopa/build05/Windows/utilities",

);

foreach my $dir(@​path)
{
  print $dir, "\n";
  $path = "$dir\\mount.exe";
  return $path if -x $path;
}

For second component of @​path, the index in vdir.h is -51 and
dirTableA array was being accessed with this index.

This is seen in all platforms of Windows and we observed the crash
with perl-5.10.1 of Active State. I checked the latest version and the
crash is observed even in that case.

-Karthik

From karthik.rajagopalan@schrodinger.com

From 56777d9085e995e6cc41020db088d02b444914ac Mon Sep 17 00:00:00 2001
From: Karthik Rajagopalan <rajagopa@pauline.schrodinger.com>
Date: Thu, 14 Jul 2011 13:29:26 -0400
Subject: [PATCH 1/2] Restrict the dirTableA access between 0 and
 drivecount-1.

In situation where PATH env is set with wrong delimiter say -

c:\\component1;c:\\component2;.:c:/component1:c:/component2

the code in perl which check if an executable exist in each component
of PATH env would crash. Though the delimiter need to be fixed, it is better
to avoid perl crash due to invalid access for wrong input.
---
 win32/vdir.h |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/win32/vdir.h b/win32/vdir.h
index db7ec84..2b36635 100644
--- a/win32/vdir.h
+++ b/win32/vdir.h
@@ -83,7 +83,15 @@ protected:
     };
     inline const char *GetDirA(int index)
     {
-	char *ptr = dirTableA[index];
+        char *ptr = NULL;
+
+        // Limit the access to dirTableA between
+        // 0 and driveCount-1. This can result
+        // in crash if PATH env is set with wrong
+        // delimiter otherwise - Karthik Rajagopalan
+	if(index >=0 && index < driveCount)
+	    ptr = dirTableA[index];
+
 	if (!ptr) {
 	    /* simulate the existance of this drive */
 	    ptr = szLocalBufferA;
-- 
1.7.5

karthik.rajagopalan@schrodinger.com - Status changed from 'new' to 'open'

karthik.rajagopalan@schrodinger.com - Status changed from 'open' to 'new'

From @steve-m-hay

Thanks for the patch. I haven't had a chance to try it yet, but from
inspection I would think the same check should definitely be added to
GetDirW(), and possibly similar checks should be added to
SetDefaultDirA() and SetDefaultDirW() before they assign index to
nDefault so that the array access in GetDefaultDirA() and
GetDefaultDirW() are safe too.

The RT System itself - Status changed from 'new' to 'open'

From rajagopa@schrodinger.com

Hi Shay,

On Wed Aug 10 14​:56​:18 2011, shay wrote​:

Thanks for the patch. I haven't had a chance to try it yet, but from
inspection I would think the same check should definitely be added to
GetDirW(), and possibly similar checks should be added to
SetDefaultDirA() and SetDefaultDirW() before they assign index to
nDefault so that the array access in GetDefaultDirA() and
GetDefaultDirW() are safe too.

Yes, I will submit the revised patch for it today.

-Karthik

From @cpansprout

On Thu Aug 11 05​:11​:36 2011, kartlee wrote​:

Hi Shay,

On Wed Aug 10 14​:56​:18 2011, shay wrote​:

Thanks for the patch. I haven't had a chance to try it yet, but from
inspection I would think the same check should definitely be added to
GetDirW(), and possibly similar checks should be added to
SetDefaultDirA() and SetDefaultDirW() before they assign index to
nDefault so that the array access in GetDefaultDirA() and
GetDefaultDirW() are safe too.

Yes, I will submit the revised patch for it today.

Did you ever get a chance to revise the patch?

From rajagopa@schrodinger.com

Hi,

Did you ever get a chance to revise the patch?

This really slipped out of my list. I am really sorry for the delay. I
will make the modifications and send you the patch by early tomorrow.

-Karthik

From rajagopa@schrodinger.com

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

From rajagopa@schrodinger.com

From 32bc7a6d53758eed943047a2ab3cfc7ef4337d02 Mon Sep 17 00:00:00 2001
From: Karthik Rajagopalan <rajagopa@francesca.schrodinger.com>
Date: Mon, 31 Oct 2011 17:24:39 -0400
Subject: [PATCH] Restrict the dirTableA and dirTableW access between 0 and
 driveCount -1.

In situation where PATH env is set with wrong delimiter say -
"c:\\component1;c:\\component2;.:c:/component1:c:/component2"

the code in perl which check if an executable exist in each component
of PATH env would crash. Though the delimiter need to be fixed, it is better
to avoid perl crash due to invalid access for wrong input.
---
 win32/vdir.h |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/win32/vdir.h b/win32/vdir.h
index db7ec84..4e6b615 100644
--- a/win32/vdir.h
+++ b/win32/vdir.h
@@ -83,7 +83,15 @@ protected:
     };
     inline const char *GetDirA(int index)
     {
-	char *ptr = dirTableA[index];
+        char *ptr = NULL;
+
+        // Limit the access to dirTableA between
+        // 0 and driveCount-1. This can result
+        // in crash if PATH env is set with wrong
+        // delimiter otherwise - Karthik Rajagopalan
+        if(index >=0 && index < driveCount)
+            ptr = dirTableA[index];
+
 	if (!ptr) {
 	    /* simulate the existance of this drive */
 	    ptr = szLocalBufferA;
@@ -96,7 +104,15 @@ protected:
     };
     inline const WCHAR *GetDirW(int index)
     {
-	WCHAR *ptr = dirTableW[index];
+        WCHAR *ptr = NULL;
+        
+        // Limit the access to dirTableW between
+        // 0 and driveCount-1. This can result
+        // in crash if PATH env is set with wrong
+        // delimiter otherwise - Karthik Rajagopalan
+        if(index >=0 && index < driveCount)
+            ptr = dirTableW[index];
+
 	if (!ptr) {
 	    /* simulate the existance of this drive */
 	    ptr = szLocalBufferW;
@@ -192,7 +208,7 @@ int VDir::SetDirA(char const *pPath, int index)
     char chr, *ptr;
     int length = 0;
     WCHAR wBuffer[MAX_PATH+1];
-    if (index < driveCount && pPath != NULL) {
+    if (index >= 0 && index < driveCount && pPath != NULL) {
 	length = strlen(pPath);
 	pMem->Free(dirTableA[index]);
 	ptr = dirTableA[index] = (char*)pMem->Malloc(length+2);
@@ -262,7 +278,7 @@ int VDir::SetDirW(WCHAR const *pPath, int index)
 {
     WCHAR chr, *ptr;
     int length = 0;
-    if (index < driveCount && pPath != NULL) {
+    if (index >= 0 && index < driveCount && pPath != NULL) {
 	length = wcslen(pPath);
 	pMem->Free(dirTableW[index]);
 	ptr = dirTableW[index] = (WCHAR*)pMem->Malloc((length+2)*2);
-- 
1.7.7.1

From @cpansprout

On Mon Oct 31 14​:33​:16 2011, kartlee wrote​:

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

Steve Hay​: Could you review the new patch in this ticket?

--

Father Chrysostomos

From [Unknown Contact. See original ticket]

On Mon Oct 31 14​:33​:16 2011, kartlee wrote​:

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

Steve Hay​: Could you review the new patch in this ticket?

--

Father Chrysostomos

From @steve-m-hay

On Mon Oct 31 14​:47​:26 2011, sprout wrote​:

On Mon Oct 31 14​:33​:16 2011, kartlee wrote​:

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

Steve Hay​: Could you review the new patch in this ticket?

Thanks for the new patch, but I had been thinking of adding extra checks
in SetDefaultDir[AW] as well so that we don't assign an out-of-bounds
value to nDefault, otherwise it would crash when using that index value
later in GetDefaultDir[AW].

However, on closer inspection, I'm not sure that the fix is ideal anyway.

I've run your example script with several different perl builds (without
your patch) and none of them crashed for me, although stepping through
it in the debugger I can see that an index of -51 is being accessed, so
it is just flukey that it didn't crash.

Stepping through it again in the debugger with your patch, it now
correctly avoids accessing index -51, but that causes it to go into this
piece of code​:

  if (!ptr) {
  /* simulate the existance of this drive */
  ptr = szLocalBufferA;
  ptr[0] = 'A' + index;
  ptr[1] = '​:';
  ptr[2] = '\\';
  ptr[3] = 0;
  }

which produces a garbage 'drive' in the ptr array (think​: which ASCII
character is 'A' + index when index is -51?!). The code scrapes through
without crashing, but it hardly seems much less flukey than it did before.

Perhaps GetDirA should just return NULL if index is out of bounds? But
then the strcpy() in MapPathA won't be happy, so that would need some
changes too... I wonder if DriveIndex also needs some work to stop it
returning an index of -51 in the first place when the path in question
begins with '.'?!

From @rjbs

Steve, do you have any news on this?

From @cpansprout

On Thu Mar 22 18​:50​:14 2012, rjbs wrote​:

Steve, do you have any news on this?

This seems like an appropriate time for me to say why I made it a
blocker to begin with.

I made it a blocker because it was a patch I couldn’t review, and it
seemed a shame it was being ignored at the time.

Since then, Steve Hay has pointed out problems with (both iterations of)
the patch, and the original poster has not come back with a corrected
version yet.

So, I don’t think it needs to block the release of 5.16.

--

Father Chrysostomos

From @steve-m-hay

On Thu Mar 22 20​:27​:26 2012, sprout wrote​:

On Thu Mar 22 18​:50​:14 2012, rjbs wrote​:

Steve, do you have any news on this?

This seems like an appropriate time for me to say why I made it a
blocker to begin with.

I made it a blocker because it was a patch I couldn’t review, and it
seemed a shame it was being ignored at the time.

Since then, Steve Hay has pointed out problems with (both iterations of)
the patch, and the original poster has not come back with a corrected
version yet.

So, I don’t think it needs to block the release of 5.16.

Sorry, I don't have any more news on this really. I was also waiting for
a revised patch from the OP, also with half a mind to take a closer look
myself if that was not forthcoming, but I haven't had a chance yet.

I agree that it shouldn't be a blocker, though. It's not a new
regression, and the crash is only the result of "garbage in" ayway.

The crash arises because the wrong path separator has been used -- "​:"
rather than ";". That leads to a string of directory paths not getting
separated, and then the whole string gets a filename appended and is
then passed to -x. And it just so happens that the first directory in
that string is the directory ".", and that character followed by the
wrong path separator character "​:" (which should have been ";", of
course) then confuses the code into believing that it's a drive letter
".​:" with a letter which it doesn't recognize ("." rather than the more
usual "C", "D", "E", etc).

The following one-liner reproduces the crash​:

perl -e "-e '.​:'"

as do plenty of other things with garbage where a drive letter was
expected, e.g.

perl -e "-e '1​:'"

but it really only arose because of the wrong path separator having been
used in the first place. Perl should handle this better, but I don't see
it as a blocker at all, especially since it isn't new.

From @rjbs

* Steve Hay via RT <perlbug-followup@​perl.org> [2012-03-23T13​:49​:17]

On Thu Mar 22 20​:27​:26 2012, sprout wrote​:

So, I don’t think it needs to block the release of 5.16.

[...]

but it really only arose because of the wrong path separator having been
used in the first place. Perl should handle this better, but I don't see
it as a blocker at all, especially since it isn't new.

Thanks, guys. Unblockered.

--
rjbs

From @jkeenan

On Fri Mar 23 10​:49​:16 2012, shay wrote​:

On Thu Mar 22 20​:27​:26 2012, sprout wrote​:

On Thu Mar 22 18​:50​:14 2012, rjbs wrote​:

Steve, do you have any news on this?

Sorry, I don't have any more news on this really. I was also waiting for
a revised patch from the OP, also with half a mind to take a closer look
myself if that was not forthcoming, but I haven't had a chance yet.

I agree that it shouldn't be a blocker, though. It's not a new
regression, and the crash is only the result of "garbage in" ayway.

The crash arises because the wrong path separator has been used -- "​:"
rather than ";". That leads to a string of directory paths not getting
separated, and then the whole string gets a filename appended and is
then passed to -x. And it just so happens that the first directory in
that string is the directory ".", and that character followed by the
wrong path separator character "​:" (which should have been ";", of
course) then confuses the code into believing that it's a drive letter
".​:" with a letter which it doesn't recognize ("." rather than the more
usual "C", "D", "E", etc).

The following one-liner reproduces the crash​:

perl -e "-e '.​:'"

as do plenty of other things with garbage where a drive letter was
expected, e.g.

perl -e "-e '1​:'"

but it really only arose because of the wrong path separator having been
used in the first place. Perl should handle this better, but I don't see
it as a blocker at all, especially since it isn't new.

I don't get these segfaults with Perl 5.16.0 on Darwin.

#####
$ perl -e "-e '.​:'"
$ perl -e "-e '1​:'"
#####

Is it possible this was a Win32-only bug?

Thank you very much.
Jim Keenan

From @bulk88

On Fri Mar 23 10​:49​:16 2012, shay wrote​:

The following one-liner reproduces the crash​:

perl -e "-e '.​:'"

as do plenty of other things with garbage where a drive letter was
expected, e.g.

perl -e "-e '1​:'"

but it really only arose because of the wrong path separator having been
used in the first place. Perl should handle this better, but I don't see
it as a blocker at all, especially since it isn't new.

Can not reproduce with ActivePerl 5.10 or 5.17.7. I ran "perl -e "-e
'1​:'"" no path changes.
--
bulk88 ~ bulk88 at hotmail.com

From @steve-m-hay

I'm not sure I ever saw it crash, actually -- but if you step through it in
the debugger you will see it doing things it shouldn't really be doing and
it seemed just a fluke that it didn't crash​: see my reply from 3 Nov 2011.

On 3 February 2013 20​:59, bulk88 via RT <perlbug-followup@​perl.org> wrote​:

On Fri Mar 23 10​:49​:16 2012, shay wrote​:

The following one-liner reproduces the crash​:

perl -e "-e '.​:'"

as do plenty of other things with garbage where a drive letter was
expected, e.g.

perl -e "-e '1​:'"

but it really only arose because of the wrong path separator having been
used in the first place. Perl should handle this better, but I don't see
it as a blocker at all, especially since it isn't new.

Can not reproduce with ActivePerl 5.10 or 5.17.7. I ran "perl -e "-e
'1​:'"" no path changes.
--
bulk88 ~ bulk88 at hotmail.com

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=94846

@steve-m-hay can this ticket be closed? The 2nd patch was provided: #11500 (comment)