"There is already an open DataReader..." can't seem to get rid of this bug

Question

So I am writing a custom membership provider for Sitecore and my strategy was to download the source code to the SQLMembershipProvider (which sitecore already uses) and just add my custom logic to it.

One of the core functionalities we wanted was the ability to store a password history and I was working in the ChangePassword function to start.

The first few times I tried my code I had no issue, but now even after computer restarts, attempting to disconnect all users from the database, and refactoring my code so that it only uses a single SQLCommand and one connection I cannot get rid of this error.

I'm including the relevant code from the change password function (I demarked the custom code I added with //my code - start/end). Any ideas?

try
{
    SqlConnectionHolder recentpw_holder = null;
    SqlDataReader reader = null;
    SqlCommand tempcmd = null;
    try
    {
        recentpw_holder = SqlConnectionHelper.GetConnection(_sqlConnectionString, true);
        CheckSchemaVersion(recentpw_holder.Connection);

        //my code - Start
        tempcmd = new SqlCommand("SELECT * FROM dbo.PasswordHistory WHERE username='" + username + "' ORDER BY date_created", recentpw_holder.Connection);

        using (reader = tempcmd.ExecuteReader())
        {
            var passwordlist = new List<string>();
            while (reader.Read())
            {
                //get the list of passwords
                for (int i = 0; i < reader.FieldCount; i++)
                {
                    passwordlist.Add(reader.GetValue(i).ToString());
                }
            }

             //if there are more than 10 passwords stored in the database remove the oldest one
             if (passwordlist.Count > 10)
             {
                 tempcmd.CommandText = "DELETE * FROM dbo.PasswordHistory" +
                     "WHERE username='" + username + "'" +
                     "AND password='" + passwordlist.First()                                  + "'";

                  tempcmd.ExecuteNonQuery();
                  passwordlist.RemoveAt(0);
              }

              //check and see if that password has already been used, if so throw an error
              foreach (var p in passwordlist)
              {
                  if (p == pass)
                  {
                      throw new ArgumentException("Unable to create password. Password does not meet the history requirements of the domain.");
                   }
               }

                }

                //my code - end

                tempcmd.CommandText = "dbo.aspnet_Membership_SetPassword";

                tempcmd.CommandTimeout = CommandTimeout;
                tempcmd.CommandType = CommandType.StoredProcedure;
                tempcmd.Parameters.Add(CreateInputParam("@ApplicationName", SqlDbType.NVarChar, ApplicationName));
                tempcmd.Parameters.Add(CreateInputParam("@UserName", SqlDbType.NVarChar, username));
                tempcmd.Parameters.Add(CreateInputParam("@NewPassword", SqlDbType.NVarChar, pass));
                tempcmd.Parameters.Add(CreateInputParam("@PasswordSalt", SqlDbType.NVarChar, salt));
                tempcmd.Parameters.Add(CreateInputParam("@PasswordFormat", SqlDbType.Int, passwordFormat));
                tempcmd.Parameters.Add(CreateInputParam("@CurrentTimeUtc", SqlDbType.DateTime, DateTime.UtcNow));

                SqlParameter para = new SqlParameter("@ReturnValue", SqlDbType.Int);
                para.Direction = ParameterDirection.ReturnValue;
                tempcmd.Parameters.Add(para);

                tempcmd.ExecuteNonQuery();

                //my code pt 2 - start
                tempcmd.Parameters.Clear();
                tempcmd.CommandType = CommandType.Text;
                tempcmd.CommandText = "INSERT INTO dbo.PasswordHistory "
                                      + "(username, password, date_created) "
                                      + "VALUES ('" +
                                      username + "', '" +
                                      pass + "', '" +
                                      DateTime.UtcNow + "')";

                tempcmd.ExecuteNonQuery();
                //my code pt 2 - end


                status =  ( ( para.Value != null ) ? ( ( int )para.Value ) : -1 );

                if ( status != 0 )
                {
                    string errText = GetExceptionText( status );

                    if ( IsStatusDueToBadPassword( status ) )
                    {
                        throw new MembershipPasswordException( errText );
                    }
                    else
                    {
                        throw new ProviderException( errText );
                    }
                }

                return true;
            }
            finally
            {
                if (reader != null)
                {
                    reader.Close();
                    reader = null;
                }

                if( recentpw_holder != null )
                {
                    recentpw_holder.Close();
                    recentpw_holder = null;
                }
            }
        } catch {
            throw;
        }

I've found it's best not to reuse SqlCommand objects. I prefer to do using (var command = new SqlCommand()) { ... } to avoid nesting commands. It looks like you have nested commands. Your sql to delete from PasswordHistory is nested. You don't mention where the error happens. However, I suspect the error happens on that command. If you move it outside of using (reader = tempcmd.ExecuteReader()) { ... } the error should go away. Also, I suggest never having SqlReader variables outside a using block. So remove SqlReader reader = null; and declare it in the using block. — Skye MacMaster, Sep 24 '15 at 17:41
And *Please* parameterize that SQL, especially when dealing with user tables, the 1st 2 SQL statements in that leave a massive security hole in your code. http://xkcd.com/327/ — Richard Seal, Sep 24 '15 at 18:23

score 0 · Answer 1 · answered Sep 24 '15 at 17:17

You need to dispose of that reader once you no longer need it. Try this:

            using (reader = tempcmd.ExecuteReader())
            {
                var passwordlist = new List<string>();
                while (reader.Read())
                {
                    //get the list of passwords
                    for (int i = 0; i < reader.FieldCount; i++)
                    {
                        passwordlist.Add(reader.GetValue(i).ToString());
                    }
                 }
            }

                //if there are more than 10 passwords stored in the database remove the oldest one
                if (passwordlist.Count > 10)
                {
                    tempcmd.CommandText = "DELETE * FROM dbo.PasswordHistory" +
                                                          "WHERE username='" + username + "'" +
                                                          "AND password='" + passwordlist.First()                                        + "'";

                    tempcmd.ExecuteNonQuery();
                    passwordlist.RemoveAt(0);
                }

                //check and see if that password has already been used, if so throw an error
                foreach (var p in passwordlist)
                {
                    if (p == pass)
                    {
                        throw new ArgumentException("Unable to create password. Password does not meet the history requirements of the domain.");
                    }
                }

"There is already an open DataReader..." can't seem to get rid of this bug

1 Answers1