VB.Net bug or Poor Coding Practice?

[John Smith]

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?

 

[Patrice]

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...

 

[Kevin Spencer]

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]

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.