VB.Net bug or Poor Coding Practice?

Discussion in 'ASP .Net' started by John Smith, Aug 11, 2005.

  1. John Smith

    John Smith Guest

    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?
    John Smith, Aug 11, 2005
    #1
    1. Advertising

  2. John Smith

    Patrice Guest

    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" <> a écrit dans le message de
    news:...
    > 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, Aug 11, 2005
    #2
    1. Advertising

  3. 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" <> wrote in message
    news:...
    > 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?
    >
    >
    >
    Kevin Spencer, Aug 11, 2005
    #3
  4. John Smith

    John Smith Guest

    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" <> wrote in message
    news:%...
    > 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" <> wrote in message
    > news:...
    >> 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?
    >>
    >>
    >>

    >
    >
    John Smith, Aug 14, 2005
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Roman Roelofsen

    coding conventions, PEP vs. practice

    Roman Roelofsen, Jan 5, 2005, in forum: Python
    Replies:
    3
    Views:
    276
    Terry Reedy
    Jan 5, 2005
  2. js
    Replies:
    6
    Views:
    322
    Mark Jeffcoat
    Nov 22, 2006
  3. Justin.Voelker

    Best Practice for page coding

    Justin.Voelker, Apr 2, 2007, in forum: HTML
    Replies:
    12
    Views:
    590
    Toby A Inkster
    Apr 4, 2007
  4. shaanxxx

    Coding practice

    shaanxxx, Dec 24, 2007, in forum: C Programming
    Replies:
    6
    Views:
    256
    Jack Klein
    Dec 24, 2007
  5. Savvoulidis Iordanis
    Replies:
    7
    Views:
    553
    Göran Andersson
    Jul 20, 2008
Loading...

Share This Page