CodeSOD: How To Ruin a Long Weekend

GRH inherited an application written by someone who is no longer employed with the company, as part of a project managed by someone who is no longer at the company, requested by an executive who is also no longer at the company. There are no documented requirements, very few tests, and a lot of "don't touch this, it works".

Well, it worked until it didn't. On a Friday before a long weekend, the application broke. As you might expect, it has a relatively small user pool, but is absolutely vital to the company's operations- if it fails, widgets don't get shipped and invoices don't get paid. So GRH got to spend that long weekend fixing the application.

In this case, the application is a Microsoft ClickOnce deployment (essentially "install without showing the user an installer") distributed through a shared network drive.

The specific section that broke was some code responsible for converting a PDF file into plain text.

 ' GRH 2021-08-25 This is bad. Probably need to revise this, depending on new deployment strategy. ' GRH 2022-01-19 Yep, bit us on the *** in Friday's deployment. Public strConverterPath As String = System.IO.Path.GetFullPath(My.Resources.ResourceManager.BaseName).Replace("bin\Debug\net6.0-windows", "").Replace("REDACTED.", "") Public Function myPDFtoTXT(ByVal strInFN As String, ByVal strOpt As String, Optional ByVal encoding As String = "") As String Dim strComm As String Dim started As Boolean myPDFtoTXT = strInFN.Replace(".pdf", ".txt") strComm = "/c """"" & strConverterPath & "\pdftotext""" & strOpt & """" & strInFN & """""" If encoding <> "" Then strComm = strComm & " -enc " & encoding End If started = myCMDprc(strComm) If Not started Then ErrMessage("conversion failed to start") endStatus = runStatus.parse_fail End If If Not System.IO.File.Exists(myPDFtoTXT) Then ErrMessage("Converstion from pdf to text failed.") End If End Function

Thanks to the comments, it's easy to see where things went wrong. The code does some string mangling on the path, expecting to remove bin\Debug\net6.0-windows and some other substring from the string. Which betrays a lot of faith in the consistency of those path names- in this case, simply building in Release mode, instead of Debug would break the application. It's also important to note that this is using the resource manager- an object whose job it is to load resource files for localization, not intended to return paths for where the application is installed (according to the docs, it doesn't even return a path, but instead the namespace of the resource file). This is emphatically the wrong tool for the job.

Based on the comments, something about the assumed paths returned by the method that isn't meant to look up paths broke. And that's the core WTF, but there's so many more things I can complain about here.

Instead of using one of the many libraries available for this task, they instead shell out to a command line tool for it. Not the worst choice, but also not my favorite thing to see. I have no idea how the implementation of myCMDprc is done, but it clearly swallows the return codes from the program being executed- instead of checking if the program exited successfully, we instead check to see if a TXT file exists, and assume that's success. Which again, puts a lot of faith in that external tool that creating the file means it succeeded, because I can imagine a lot of scenarios where it starts flushing the buffers but also doesn't successfully generate correct output.

Finally, this also uses a perfectly valid VB.Net idiom, but also one I hate- it treats the function name itself as a variable. myPDFtoTXT is assigned a value, which will be the return value of the function. Later on, for checking that the output file exists, it uses the function's return value as a variable. This isn't technically wrong, I just hate it.

And I lied about that being the last WTF- it's the last WTF the programmer put there, but the final WTF is just VB's approach to escaping the " character in strings- three double quote characters in a row is a single double quote character in the output: `""""Hello world,""" they said."

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!