CODEWITHSUNDEEP
c++clabelgoto
SSpoke
SSpoke

Reputation: 5836

How can I refactor this C++ to remove the labels/gotos?

My problem is not really a problem, but I want to make this code look a bit more elegant than it currently is. It has spaghetti code.

Here is a little example of what I got.

 int func(...) {
  if ( ... )
  {
    v6 = ESI_1C;
    EBP = 1;
    v5 = 0;
    v21 = 0;
    v19 = 0;
    v25 = ESI_1C;
    if ( packetSize > 1 )
    {
      v26 = (unsigned __int8)initialCryptAnswer;
      v10 = v8 - v6;
      v11 = v6 + 4;
      for ( i = v8 - v6; ; v10 = i )
      {
        v12 = *(DWORD *)(v10 + v11);
        v21 += v12;
        cryptAnswer = (unsigned __int8)cryptTable[(2 * (unsigned __int8)v26)+1];
        v13 = EBP & 3;
        if ( !(EBP & 3) )
          break;
        if ( v13 == 1 ) {
          v16 = cryptAnswer >> 1;
          *(DWORD *)v11 = v12 - v16;
        } else if ( v13 == 2 ) {
          *(DWORD *)v11 = v12 + 2 * cryptAnswer;
        } else if ( v13 == 3 ) {
          v16 = cryptAnswer >> 2;
          *(DWORD *)v11 = v12 - v16;
        }
LABEL_19:
        v17 = *(DWORD *)v11 + v19;
        ++EBP;
        v11 += 4;
        v19 = v17;
        ++v26;
        if ( EBP >= packetSize )
        {
          ESI = v24;
          v5 = v17;
          v6 = v25;
          goto LABEL_22; //kinda like break it's much more because it's instead of a for loop too.
        }
      }
      *(DWORD *)v11 = v12 + 4 * cryptAnswer;
      goto LABEL_19; //this is a looper
    }
LABEL_22:
    result = 1;
  } else {
    result = 0;
  }
  return result;
}

What can I do to get rid of the labels without messing up the code flow as it's very important part of an encryption function?

Upvotes: 2

Views: 1073

Answers (4)

i0h3
i0h3

Reputation: 1

The answer is to remove the break statement and nest the if/else if blocks checking v13==1 v13==2 and v13==3inside a new if statement checking true rather than false, with an else statement to run the assignment before goto LABEL_19 and remove the labels and goto statements.

if(EBP & 3){
    if (v13==1){
        v16 = cryptAnswer >> 1;
        *(DWORD *)v11 = v12 - v16;
    }
    if (v13==2){
        *(DWORD *)v11 = v12 + 2 * cryptAnswer;
    }
    if (v13==3){
        v16 = cryptAnswer >> 2;
        *(DWORD *)v11 = v12 - v16;
    }
} else {*(DWORD *)v11 = v12 + 4 * cryptAnswer;}

The break statement is there ONLY to skip the equivalency checks on v13, which is possible with a decision structure alone. Equivalency checks do not need else if statements, which is why my solution eliminates them. I'm answering this because the person who asked the question let the answer be perceived as correct, and most of it is correct, but the first example is incorrect and a reason isn't given for why the second example works.

Upvotes: 0

Lindydancer
Lindydancer

Reputation: 26094

Second answer, as the question changed a lot.

The original code looks like, after removing all redundant stuff:

for (... ; ; ...)
{
  ...
  if (...)
  {
    break;
  }
  ABC
LABEL_19:
  DEF
}
XYZ;
goto LABEL_19;

This is exactly the same as:

for (... ; ; ...)
{
  ...
  if (...)
  {
    XYZ;
  }
  else
  {
    ABC
  }

  DEF
}

Once you have done this rewrite, you could replace the goto LABEL_22 with a simple break.

EDIT: Or more explicitly:

for (... ; ; ...)
{
  ...
  if (! (ESP & 3) )
  {
    // This line used to be located after the loop.
    *(DWORD *)v11 = v12 + 4*cryptAnswer;  // 111
  }
  else
  {
    if (v13 == 1)
    {
      ...
    }
    else if (v13 == 2)
    {
      ...
    }
    else if (v13 == 3)
    {
      ...
    }
  }

  // This is where LABEL_19 used to be.

  v17 = ...      // 222
}

The rule of thumb when you rewrite code like this is that the code should perform the same actions in the same order as before. You simply can't move code around blindly, and to do this you must understand the flow of the code. In this case, when the first if is taken, the line I've marked with // 111 is first executed, then the line // 222, nothing else.

Upvotes: 2

Patrick
Patrick

Reputation: 8320

It would be better if you cut and pasted the code so we can cut and paste it into an editor but basically the line before goto Label_19 needs to go where the break line is. The if statement following the break needs to become an else if and the goto label 22 can become a break (or move the conditional that leads to goto label 22 into the for loop as in my previous answer) into the for loop.

PS: your secret isn't a secret to anyone who has the executable

Upvotes: 1

Lindydancer
Lindydancer

Reputation: 26094

The goto LABEL_1 is never executed, as there is no way out of the for loop.

Once this has been removed, you can replace goto LABEL_2 with a break, or even better simply return 1; (or result = 1; return 1; if return is not a local variable.)

Upvotes: 1

Related Questions