CodeSOD: Trophy Bug Hunting

Quality control is an important business function for any company. When your company is shipping devices with safety concerns, it's even more important. In some industries, a quality control failure is bound to be national headlines.

When the quality control software tool stopped working, everyone panicked. At which point, GRH stepped in.

Now, we've discussed this software and GRH before, but as a quick recap, it was:

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

And this was a quality control tool. So we're already in bad shape. It also had been unmaintained for years- a few of the QC engineers had tried to take it over, but weren't programmers, and it had essentially languished.

Specifically, it was a quality control tool used to oversee the process by about 50 QC engineers. It automates a series of checks by wrapping around third party software tools, in a complex network of "this device gets tested by generating output in program A, feeding it to program B, then combining the streams and sending them to the device, but this device gets tested using programs D, E, and F."

The automated process using the tool has a shockingly low error rate. Without the tool, doing things manually, the error rate climbs to 1-2%. So unless everyone wanted to see terrifying headlines in the Boston Globe about their devices failing, GRH needed to fix the problem.

GRH was given the code, in this case a a zip file on a shared drive. It did not, at the start, even build. After fighting with the project configuration to resolve that, GRH was free to start digging in deeper.

Public Sub connect2PCdb() Dim cPath As String = Path.Combine(strConverterPath, "c.pfx") Dim strCN As String ' JES 12/6/2016: Modify the following line if MySQL server is changed to a different server. A dump file will be needed to re-create teh database in the new server. strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;database=REDACTED;sslmode=Required;certificatepassword=REDACTED;certificatefile=REDACTED\c.pfx;password=REDACTED'" strCN = Regex.Replace(strCN, "certificatefile=.*?pfx", "certificatefile=" & cPath) pcContext = New Entities(strCN) strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;persistsecurityinfo=True;database=REDACTED;password=REDACTED'" strCN = Regex.Match(strCN, ".*'(.*)'").Groups(1).Value Try strCN = pcContext.Database.Connection.ConnectionString cnPC.ConnectionString = "server=REDACTED;user id=REDACTED;password=REDACTED;database=REDACTED;" cnPC.Open() Catch ex As Exception End Try End Sub

This is the code which connects to the backend database. The code is in the category of more of a trainwreck than a WTF. It's got a wonderful mix of nonsense in here, though- a hard-coded connection string which includes plaintext passwords, regex munging to modify the string, then hard-coding a string again, only to use regexes to extract a subset of the string. A subset we don't use.

And then, for a bonus, the whole thing has a misleading comment- "modify the following line" if we move to a different server? We have to modify several lines, because we keep copy/pasting the string around.

Oh, and of course, it uses the pattern of "open a database connection at application startup, and just hold that connection forever," which is a great way to strain your database as your userbase grows.

The good news about the hard-coded password is that it got GRH access to the database. With that, it was easy to see what the problem was: the database was full. The system was overly aggressive with logging, the logs went to database tables, the server was an antique with a rather small hard drive, and the database wasn't configured to even use all of that space anyway.

Cleaning up old logs got the engineers working again. GRH kept working on the code, though, cleaning it up and modernizing it. Updating to latest version of the .NET Core framework modified the data access to be far simpler, and got rid of the need for hard-coded connection strings. Still, GRH left the method looking like this:

 Public Sub connect2PCdb() 'Dim cPath As String = Path.Combine(strConverterPath, "c.pfx") 'Dim strCN As String ' JES 12/6/2016: Modify the following line if MySQL server is changed to a different server. A dump file will be needed to re-create teh database in the new server. 'strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;database=REDACTED;sslmode=Required;certificatepassword=REDACTED;certificatefile=REDACTED\c.pfx;password=REDACTED'" 'strCN = Regex.Replace(strCN, "certificatefile=.*?pfx", "certificatefile=" & cPath) 'pcContext = New Entities(strCN) 'strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;persistsecurityinfo=True;database=REDACTED;password=REDACTED'" 'strCN = Regex.Match(strCN, ".*'(.*)'").Groups(1).Value 'GRH 2021-01-15. Connection information moved to App.Config 'GRH 2021-08-13. EF Core no longer supports App.Config method pcContext = New PcEntities Try ' GRH 2021-08-21 This variable no longer exists in .NET 5 'strCN = pcContext.Database.Connection.ConnectionString ' GRH 2021-08-20 Keeping the connection open causes EF Core to not work 'cnPC.ConnectionString = "server=REDACTED;user id=REDACTED;password=REDACTED;database=REDACTED;SslMode=none" 'cnPC.Open() Catch ex As Exception End Try End Sub

It's now a one-line method, with most of the code commented out, instead of removed. Why on Earth is the method left like that?

GRH explains:

Yes, I could delete the function as it is functionally dead, but I keep it for the same reasons that a hunter mounts a deer's head above her mantle.

[Advertisement] Plan Your .NET 9 Migration with Confidence
Your journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!