Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > ASP .Net > VB.Net bug or Poor Coding Practice?

Reply
Thread Tools

VB.Net bug or Poor Coding Practice?

 
 
John Smith
Guest
Posts: n/a
 
      08-11-2005
Here's the scenario...

First let me say that the following events were catastrophic to my
development workstation because my C:\WINNT directory had the EVERYONE user
group assigned with full control (inherited by all sub-directories). I
believe this was set inadvertantly some time in the past while debugging a
permissions issue. I know that this is very wrong, but it's not the problem
I'm questioning.

We have a ASP.Net page (VB Code-behind) that binary writes an Excel file
that is created by an external component. The path to the file is passed in
via the querystring on a redirect. Once the page builds a byte array
containing the Excel file, the temp directory containing the physical file
is deleted. By design, we create the Excel file within a subdirectory of
C:\temp that is named with a GUID. When we are through with the file we
delete the GUID directory and all files it contains.

The code looks like this (abbreviated for clarity):

1 Protected Function ReturnExcelFile(ByVal sFilePathName As String, _
2 ByRef arExcelBytes() As Byte) As Boolean
3 Dim oFileStream As FileStream
4 Dim oStreamReader As StreamReader
5 Dim sError As String
6 Dim i As Integer
7
8 Dim oExcelFileInfo As New FileInfo(sFilePathName)
9
10 If Not oExcelFileInfo.Exists Then
11 sError = "Expected Excel file does not exist."
12 GoTo ErrorExit
13 End If
14
15 <<<BUILD BYTE ARRAY LOGIC>>>
16
17 'Clean up the File
18 Try
19 oExcelFileInfo.Directory.Delete(True)
20 Catch excp As Exception
21 sError = "Error: " & Err.Number & " " & Err.Description
22 GoTo ErrorExit
23 End Try
24
25 Return True
26
27 ErrorExit:
28 'Clean up the Directory and File
29 Try
30 oExcelFileInfo.Directory.Delete(True)
31 Catch excp As Exception
32 'Nothing
33 End Try
34
35 sQueryError.Value = "An error occurred creating the Excel file." &
vbCrLf & vbCrLf & _
36 "If the problem persists, contact the Support Center." &
vbCrLf & vbCrLf & _
37 sError
38
39 Return False
40 End Function
End Class

A recent change resulted in an error that removed the path from the variable
sFilePathName. Instead of passing "C:\Temp\guid\myfile.xls", sFilePathName
ended up as "C__Temp_guid_myfile.xls". Line 8 above creates a new FileInfo
object, oExcelFileInfo using sFilePathName as the parameter. Line 10 checks
to see if the file exists. If it doesn't (as was the case after the error
was introduced), we jump down to line 27 to clean up the temp directory.
Line 30 deletes the Directory within oExcelFileInfo and all objects within
it.

Here's where it turned nasty for me.

When I passed "C__Temp_guid_myfile.xls" into the function, the file did not
exist and we jumped to the ErrorExit label. Keep in mind that oExcelFileInfo
does not point to a valid file. One would expect that oExcelFile.Directory
would not point to a valid directory either (hence the try with no exception
thrown in the catch), right? WRONG! In this case, oExcelFileInfo.Directory
points to C:\WINNT\system32. As you can imagine when the EVERYONE group has
full control to the WINNT\system32 directory, performing a recursive delete
is not a good thing!

So... I know that the permissions part was a BIG mistake on my part, but WHY
oh WHY would oExcelFileInfo.Directory point to the system directory when you
instantiate it with an invalid path/file? Is this a bug or should I be
taking a different approach here?



 
Reply With Quote
 
 
 
 
Patrice
Guest
Posts: n/a
 
      08-11-2005
The directory is computed from the file name and as you have no path
specification it finally just resolves to the current directory whatever it
is.

You could perhaps handle separately the working directory and the filename.
It would help ensuring that file operations are taken place in the targetted
directory.
Plus you could perhaps create unique filenames in a directory rather than
creating and deleting unique directories.
You could perhaps check for file existance and then create the FileInfo.
This way you are sure that FileInfo is valid...

--
Patrice

"John Smith" <(E-Mail Removed)> a écrit dans le message de
news:(E-Mail Removed)...
> Here's the scenario...
>
> First let me say that the following events were catastrophic to my
> development workstation because my C:\WINNT directory had the EVERYONE

user
> group assigned with full control (inherited by all sub-directories). I
> believe this was set inadvertantly some time in the past while debugging a
> permissions issue. I know that this is very wrong, but it's not the

problem
> I'm questioning.
>
> We have a ASP.Net page (VB Code-behind) that binary writes an Excel file
> that is created by an external component. The path to the file is passed

in
> via the querystring on a redirect. Once the page builds a byte array
> containing the Excel file, the temp directory containing the physical file
> is deleted. By design, we create the Excel file within a subdirectory of
> C:\temp that is named with a GUID. When we are through with the file we
> delete the GUID directory and all files it contains.
>
> The code looks like this (abbreviated for clarity):
>
> 1 Protected Function ReturnExcelFile(ByVal sFilePathName As String, _
> 2 ByRef arExcelBytes() As Byte) As Boolean
> 3 Dim oFileStream As FileStream
> 4 Dim oStreamReader As StreamReader
> 5 Dim sError As String
> 6 Dim i As Integer
> 7
> 8 Dim oExcelFileInfo As New FileInfo(sFilePathName)
> 9
> 10 If Not oExcelFileInfo.Exists Then
> 11 sError = "Expected Excel file does not exist."
> 12 GoTo ErrorExit
> 13 End If
> 14
> 15 <<<BUILD BYTE ARRAY LOGIC>>>
> 16
> 17 'Clean up the File
> 18 Try
> 19 oExcelFileInfo.Directory.Delete(True)
> 20 Catch excp As Exception
> 21 sError = "Error: " & Err.Number & " " & Err.Description
> 22 GoTo ErrorExit
> 23 End Try
> 24
> 25 Return True
> 26
> 27 ErrorExit:
> 28 'Clean up the Directory and File
> 29 Try
> 30 oExcelFileInfo.Directory.Delete(True)
> 31 Catch excp As Exception
> 32 'Nothing
> 33 End Try
> 34
> 35 sQueryError.Value = "An error occurred creating the Excel file."

&
> vbCrLf & vbCrLf & _
> 36 "If the problem persists, contact the Support Center." &
> vbCrLf & vbCrLf & _
> 37 sError
> 38
> 39 Return False
> 40 End Function
> End Class
>
> A recent change resulted in an error that removed the path from the

variable
> sFilePathName. Instead of passing "C:\Temp\guid\myfile.xls", sFilePathName
> ended up as "C__Temp_guid_myfile.xls". Line 8 above creates a new

FileInfo
> object, oExcelFileInfo using sFilePathName as the parameter. Line 10

checks
> to see if the file exists. If it doesn't (as was the case after the error
> was introduced), we jump down to line 27 to clean up the temp directory.
> Line 30 deletes the Directory within oExcelFileInfo and all objects within
> it.
>
> Here's where it turned nasty for me.
>
> When I passed "C__Temp_guid_myfile.xls" into the function, the file did

not
> exist and we jumped to the ErrorExit label. Keep in mind that

oExcelFileInfo
> does not point to a valid file. One would expect that oExcelFile.Directory
> would not point to a valid directory either (hence the try with no

exception
> thrown in the catch), right? WRONG! In this case, oExcelFileInfo.Directory
> points to C:\WINNT\system32. As you can imagine when the EVERYONE group

has
> full control to the WINNT\system32 directory, performing a recursive

delete
> is not a good thing!
>
> So... I know that the permissions part was a BIG mistake on my part, but

WHY
> oh WHY would oExcelFileInfo.Directory point to the system directory when

you
> instantiate it with an invalid path/file? Is this a bug or should I be
> taking a different approach here?
>
>
>



 
Reply With Quote
 
 
 
 
Kevin Spencer
Guest
Posts: n/a
 
      08-11-2005
Hi John,

You did make a number of mistakes. You are, of course, cognizant of the
first mistake, which was giving Everyone full control of your C:\WINNT
directory. I can't imagine any scenario that would dictate doing that, but
there you are.

Your second mistake was passing the full path to the temp directory to the
method. It would have been wiser to define a parent directory path
("C:\Temp") for the temp directory, and use System.IO.Path.Combine() to
build the fully-qualified path to the temp directory under it. Besides good
application design (making the temp directory configurable), it would have
prevented this or any similar scenario from occurring.

I'm not sure whether this was a mistake or not, but for some reason, your
code seems to indicate that if the file is not found, the directory
containing it should be deleted. I can't see any logic behind this. But I
don't know what that GUID directory might have been used for in other code
in your app.

Using a GOTO is bad form in general, and should be avoided. That is, after
all, what functions are for.

As to why it deleted the C:\WINNT\system32 directory,when you pass in a
relative path (not a fully-qualified path), the FileInfo assumes the
directory that the app is executing in. The asp.net worker process runs in
the C:\WINNT\system32 directory. Hence, adios C:\WINNT\system32. Again,
using a configurable path to the parent C:\Temp directory, and
System.IO.Path.Combine() to build the fully-qualified path would have
prevented this.

--
HTH,

Kevin Spencer
Microsoft MVP
..Net Developer
Everybody picks their nose,
But some people are better at hiding it.

"John Smith" <(E-Mail Removed)> wrote in message
news:(E-Mail Removed)...
> Here's the scenario...
>
> First let me say that the following events were catastrophic to my
> development workstation because my C:\WINNT directory had the EVERYONE
> user group assigned with full control (inherited by all sub-directories).
> I believe this was set inadvertantly some time in the past while debugging
> a permissions issue. I know that this is very wrong, but it's not the
> problem I'm questioning.
>
> We have a ASP.Net page (VB Code-behind) that binary writes an Excel file
> that is created by an external component. The path to the file is passed
> in via the querystring on a redirect. Once the page builds a byte array
> containing the Excel file, the temp directory containing the physical file
> is deleted. By design, we create the Excel file within a subdirectory of
> C:\temp that is named with a GUID. When we are through with the file we
> delete the GUID directory and all files it contains.
>
> The code looks like this (abbreviated for clarity):
>
> 1 Protected Function ReturnExcelFile(ByVal sFilePathName As String, _
> 2 ByRef arExcelBytes() As Byte) As Boolean
> 3 Dim oFileStream As FileStream
> 4 Dim oStreamReader As StreamReader
> 5 Dim sError As String
> 6 Dim i As Integer
> 7
> 8 Dim oExcelFileInfo As New FileInfo(sFilePathName)
> 9
> 10 If Not oExcelFileInfo.Exists Then
> 11 sError = "Expected Excel file does not exist."
> 12 GoTo ErrorExit
> 13 End If
> 14
> 15 <<<BUILD BYTE ARRAY LOGIC>>>
> 16
> 17 'Clean up the File
> 18 Try
> 19 oExcelFileInfo.Directory.Delete(True)
> 20 Catch excp As Exception
> 21 sError = "Error: " & Err.Number & " " & Err.Description
> 22 GoTo ErrorExit
> 23 End Try
> 24
> 25 Return True
> 26
> 27 ErrorExit:
> 28 'Clean up the Directory and File
> 29 Try
> 30 oExcelFileInfo.Directory.Delete(True)
> 31 Catch excp As Exception
> 32 'Nothing
> 33 End Try
> 34
> 35 sQueryError.Value = "An error occurred creating the Excel file."
> & vbCrLf & vbCrLf & _
> 36 "If the problem persists, contact the Support Center." &
> vbCrLf & vbCrLf & _
> 37 sError
> 38
> 39 Return False
> 40 End Function
> End Class
>
> A recent change resulted in an error that removed the path from the
> variable sFilePathName. Instead of passing "C:\Temp\guid\myfile.xls",
> sFilePathName ended up as "C__Temp_guid_myfile.xls". Line 8 above creates
> a new FileInfo object, oExcelFileInfo using sFilePathName as the
> parameter. Line 10 checks to see if the file exists. If it doesn't (as was
> the case after the error was introduced), we jump down to line 27 to clean
> up the temp directory. Line 30 deletes the Directory within oExcelFileInfo
> and all objects within it.
>
> Here's where it turned nasty for me.
>
> When I passed "C__Temp_guid_myfile.xls" into the function, the file did
> not exist and we jumped to the ErrorExit label. Keep in mind that
> oExcelFileInfo does not point to a valid file. One would expect that
> oExcelFile.Directory would not point to a valid directory either (hence
> the try with no exception thrown in the catch), right? WRONG! In this
> case, oExcelFileInfo.Directory points to C:\WINNT\system32. As you can
> imagine when the EVERYONE group has full control to the WINNT\system32
> directory, performing a recursive delete is not a good thing!
>
> So... I know that the permissions part was a BIG mistake on my part, but
> WHY oh WHY would oExcelFileInfo.Directory point to the system directory
> when you instantiate it with an invalid path/file? Is this a bug or should
> I be taking a different approach here?
>
>
>



 
Reply With Quote
 
John Smith
Guest
Posts: n/a
 
      08-14-2005
All points well taken. There are reasons that led to the code design that's
currently in place, but it obviously needs to be reworked.

Thanks for the feedback.

"Kevin Spencer" <(E-Mail Removed)> wrote in message
news:%(E-Mail Removed)...
> Hi John,
>
> You did make a number of mistakes. You are, of course, cognizant of the
> first mistake, which was giving Everyone full control of your C:\WINNT
> directory. I can't imagine any scenario that would dictate doing that, but
> there you are.
>
> Your second mistake was passing the full path to the temp directory to the
> method. It would have been wiser to define a parent directory path
> ("C:\Temp") for the temp directory, and use System.IO.Path.Combine() to
> build the fully-qualified path to the temp directory under it. Besides
> good application design (making the temp directory configurable), it would
> have prevented this or any similar scenario from occurring.
>
> I'm not sure whether this was a mistake or not, but for some reason, your
> code seems to indicate that if the file is not found, the directory
> containing it should be deleted. I can't see any logic behind this. But I
> don't know what that GUID directory might have been used for in other code
> in your app.
>
> Using a GOTO is bad form in general, and should be avoided. That is, after
> all, what functions are for.
>
> As to why it deleted the C:\WINNT\system32 directory,when you pass in a
> relative path (not a fully-qualified path), the FileInfo assumes the
> directory that the app is executing in. The asp.net worker process runs in
> the C:\WINNT\system32 directory. Hence, adios C:\WINNT\system32. Again,
> using a configurable path to the parent C:\Temp directory, and
> System.IO.Path.Combine() to build the fully-qualified path would have
> prevented this.
>
> --
> HTH,
>
> Kevin Spencer
> Microsoft MVP
> .Net Developer
> Everybody picks their nose,
> But some people are better at hiding it.
>
> "John Smith" <(E-Mail Removed)> wrote in message
> news:(E-Mail Removed)...
>> Here's the scenario...
>>
>> First let me say that the following events were catastrophic to my
>> development workstation because my C:\WINNT directory had the EVERYONE
>> user group assigned with full control (inherited by all sub-directories).
>> I believe this was set inadvertantly some time in the past while
>> debugging a permissions issue. I know that this is very wrong, but it's
>> not the problem I'm questioning.
>>
>> We have a ASP.Net page (VB Code-behind) that binary writes an Excel file
>> that is created by an external component. The path to the file is passed
>> in via the querystring on a redirect. Once the page builds a byte array
>> containing the Excel file, the temp directory containing the physical
>> file is deleted. By design, we create the Excel file within a
>> subdirectory of C:\temp that is named with a GUID. When we are through
>> with the file we delete the GUID directory and all files it contains.
>>
>> The code looks like this (abbreviated for clarity):
>>
>> 1 Protected Function ReturnExcelFile(ByVal sFilePathName As String, _
>> 2 ByRef arExcelBytes() As Byte) As Boolean
>> 3 Dim oFileStream As FileStream
>> 4 Dim oStreamReader As StreamReader
>> 5 Dim sError As String
>> 6 Dim i As Integer
>> 7
>> 8 Dim oExcelFileInfo As New FileInfo(sFilePathName)
>> 9
>> 10 If Not oExcelFileInfo.Exists Then
>> 11 sError = "Expected Excel file does not exist."
>> 12 GoTo ErrorExit
>> 13 End If
>> 14
>> 15 <<<BUILD BYTE ARRAY LOGIC>>>
>> 16
>> 17 'Clean up the File
>> 18 Try
>> 19 oExcelFileInfo.Directory.Delete(True)
>> 20 Catch excp As Exception
>> 21 sError = "Error: " & Err.Number & " " & Err.Description
>> 22 GoTo ErrorExit
>> 23 End Try
>> 24
>> 25 Return True
>> 26
>> 27 ErrorExit:
>> 28 'Clean up the Directory and File
>> 29 Try
>> 30 oExcelFileInfo.Directory.Delete(True)
>> 31 Catch excp As Exception
>> 32 'Nothing
>> 33 End Try
>> 34
>> 35 sQueryError.Value = "An error occurred creating the Excel
>> file." & vbCrLf & vbCrLf & _
>> 36 "If the problem persists, contact the Support Center." &
>> vbCrLf & vbCrLf & _
>> 37 sError
>> 38
>> 39 Return False
>> 40 End Function
>> End Class
>>
>> A recent change resulted in an error that removed the path from the
>> variable sFilePathName. Instead of passing "C:\Temp\guid\myfile.xls",
>> sFilePathName ended up as "C__Temp_guid_myfile.xls". Line 8 above
>> creates a new FileInfo object, oExcelFileInfo using sFilePathName as the
>> parameter. Line 10 checks to see if the file exists. If it doesn't (as
>> was the case after the error was introduced), we jump down to line 27 to
>> clean up the temp directory. Line 30 deletes the Directory within
>> oExcelFileInfo and all objects within it.
>>
>> Here's where it turned nasty for me.
>>
>> When I passed "C__Temp_guid_myfile.xls" into the function, the file did
>> not exist and we jumped to the ErrorExit label. Keep in mind that
>> oExcelFileInfo does not point to a valid file. One would expect that
>> oExcelFile.Directory would not point to a valid directory either (hence
>> the try with no exception thrown in the catch), right? WRONG! In this
>> case, oExcelFileInfo.Directory points to C:\WINNT\system32. As you can
>> imagine when the EVERYONE group has full control to the WINNT\system32
>> directory, performing a recursive delete is not a good thing!
>>
>> So... I know that the permissions part was a BIG mistake on my part, but
>> WHY oh WHY would oExcelFileInfo.Directory point to the system directory
>> when you instantiate it with an invalid path/file? Is this a bug or
>> should I be taking a different approach here?
>>
>>
>>

>
>



 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Poor, poor P&S owner learns too late... Rich Digital Photography 66 06-11-2009 04:48 AM
*bug* *bug* *bug* David Raleigh Arnold Firefox 12 04-02-2007 03:13 AM
Poor reception, poor connection, and dropped signal =?Utf-8?B?dW51c3VhbHBzeWNobw==?= Wireless Networking 2 06-07-2006 12:54 AM
general coding issues - coding style... calmar Python 11 02-21-2006 10:36 AM
wireless signal strength is really poor? Devin Panchal Wireless Networking 2 07-20-2004 12:40 AM



Advertisments