WinBox v3.43 ESR (bugfixes, improvements)

Since MikroTik has abandoned this great app and refuses to fix anything, while forcing to use their new half-baked product, will try to do some fixes and improvements on my own. Nothing radical, just some little polishing, because such work requires using disassembler, analyzing the code on a low level and patching it, it's not very easy and even a small change takes a lot of time. I'm not super-experienced with that, but decided to try some things.


So, below is the first custom version, that fixes a multiyear bug with grouping of thousands for numbers from 10^9 and larger. This occurs in 'Interfaces' and 'Firewall' windows for packet counters, and instead of 1 234 567 890 WinBox was showing 1234 567 890. The original conversion code was fully replaced by my own code, and it works correctly with all numbers up to the highest value limit (2^64 - 1).

Result:

Even though it's a very small change, it took almost 2 days to figure out and fix it. Probably, advanced hackers could do this for 2 hours or even quicker :slightly_smiling_face:

Also, modified version info:

:warning: Obviously, digital signature of the file is not valid anymore.

:link: Download: winbox64_ESR_1.zip (761.3 KB)

Description for advanced users

The original function, that was converting numbers is located at address 0x46BEFC (offset in .exe file is 0x0006B323). It was implemented in a very weird way. First, they are comparing a number with 999. If it's not greater, they convert it as is using sprintf function. If a number is larger, then they compare it with 999 999. In case it's not greater they divide it by 1000 and pass 2 numbers (quotient and remainder) to sprintf function, that adds space between them using format string. In case a number is greater than 999 999, they also divide it by 1 000 000 and pass 3 numbers to sprintf function. But this third number (most significant part) is left as is and not grouped. So, they simply don't care about larger numbers and maximum 3 groups can be displayed. Won't comment this implementation...

My own conversion code works simpler and just divides the number by 10 to extract every digit. At the same time it counts, how many digits were processed and adds a space after each 3 converted digits.

The code is below (x64 assembler):

  lea r13, [rsp+30h]        // Load string buffer (address was taken from the original code)

  add r13, 27               // Add offset, because conversion is done from end to start
  mov byte ptr [r13], 0     // Null-character

  mov r11, 4                // Digits counter for grouping (+1 for the first iteration)

  mov rax, r9               // Number is located in R9
  mov r9, 10                // Divider

@ConvCycle:
  dec r11                   // Decrease digits counter for grouping
  jnz @SkipGroup
  mov r11, 3                // If 3 symbols moved to buffer,
  dec r13
  mov byte ptr [r13], $20   // then add 'Space' character
@SkipGroup:
  mov rdx, 0
  div r9                    // Divide by 10, RAX - quotient, RDX - remainder
  add rdx, $30              // Convert to digit character
  dec r13
  mov byte ptr [r13], dl    // Move character to buffer

  test rax, rax             // Check if a number is fully converted
  ja @ConvCycle
  cmp byte ptr [r8], 0      // R8 contains sign
  je @End
  dec r13
  mov byte ptr [r13], '-'
@End:
  jmp  0x46BFB1             // Continue to the original code
  nop
  nop
  ...

This assembly code was converted into machine code and placed instead of original code. It's smaller than original code, so all remaining bytes were filled with NOP instruction (0x90). You may check .exe by yourself in some HEX editor or disassembler.


If you have any ideas to fix some issues, feel free to share, but I can't guarantee to fix them.

I've written this before, and now I'll say it more clearly:
If RouterOS and WinBox were open source, and some members of this forum were working on it,
they would have been "near" perfect already years ago...

It won't happen, RouterOS and winbox protocol are proprietary, and MikroTik is earning money with that.

BUT, separating networking code of WinBox into DLL and open-sourcing GUI part would be great. They won't disclose any sensitive code and others could have an ability to improve interface, though in a limited way.

Nope, will never be. Like all opensource GUI projects, no one will fully be pleased by it, it will have some bizarre GUI decisions made by a few dictatorial main contributors, some obscure features that almost no one uses, some Frankenstein mix-ups of different UI guidelines and someone's controversial personal preferences :rofl:

It wasn't so much to make them something else, but to correct all the mistakes that are there...

You realize that... basically... you described WinBox 4???....

Mikrorik is earning money with WinBox?

This implementation is junior level number formatting logic.

Hmmm, the intern that wrote It must have been later promoted to "Winbox 4 Chief Architect" ...

I have no idea if disassembling binary really can "reveal" how the logic was actually implemented in the higher language. Is it really possible to tell? There is a compiler that does optimizations and so on.

No, with RouterOS. As a licenses and as a part of router's cost. WinBox protocol is also a part of ROS. That's why I said, they won't open-source it.


If you understand assembler, this logic is visible, the code is short and not very complex.

On the image in the block for Value > 999999 1st, 2nd and 3rd parameter actually means 4th, 5th and 6th parameter accordingly, I just didn't take first three into account, when commenting the code.

So, on high-level it looks like this:

if (Value > 999) {
  if (Value > 999999) {
    sprintf(StrBuffer, "%s%u %3.3u %3.3u", Sign, Value / 1000000, (Value % 1000000) / 1000, Value % 1000);
  } else {
    sprintf(StrBuffer, "%s%u %3.3u", Sign, Value / 1000, Value % 1000);
  }
} else {
  sprintf(StrBuffer, "%s%u", Sign, Value);
}

If they are also coding ROS like this no wonder that there is huge bug list…

Don't be so judgemental. That code could be there since at least two decades.
It's a small binary that did a great job for many many many years.

The problem is that they didn't fix it for years. I've created a ticket about this bug in WinBox 3 (!) more than 3 years ago, when WinBox 4 doesn't even exist. After these years the ticket has received a message, that it was "fixed". In WinBox 4, not in WinBox 3 that it was related to!
I've fixed it in 2 days without sources, they couldn't fix it in 3 years. WTF...

Oh, that's another story.
I've reported the use of Inconsistent units between management utilities (android app vs winbox/webfig) using MB vs MiB for example, obviously showing different values if you look at both of them at the same time and you need to take into account that the numbers mean something else in each of those screens.
Some shown values use 32bit unsigned int variables that cause overflow over 4GB, or even 32bit signed that causes that value to be shown as negative after it overflows, funky that last one while watch data used of a wireguard peer.
But these bugs while annoying to be there for years (years MikroTik, years!) they are considered cosmetic bugs almost everywhere, and not much effort (money) is put into fixing them, not onlly at MikroTik.

All these years, we thought MikroTik rejected requests to make Winbox open source because of company compliance reasons. However, it seems more likely that they wanted to hide the poor quality of their code.

Now, now, besides having a bit of fun on this, it can well be that it comes from a copy/paste of very old code that noone noticed/reviewed and was simply brought over from times where 640 kb were enough for everyone ...
As I see the issue is not in the code itself, as the consequences are not that much relevant, but rather in the fact that it was reported and in over three years there were not either:
a. the resources
or
b. the will
to spend - what, two or three hours? - to fix this little thing.

Item #2642 :wink: in the todo list should anywyay - before or later - be done, and three years sounds like an appropriate time to go throu the first 2641 items.

I am not a programmer, so I cant read nor understand it. It seems the TO complained about how Winbox3 grouped long numbers? I always tought Winbox displays such things in accordance with the OS settings:

It seems not, I never noticed this for about 20 years.

It makes no sense to format numbers this way. My C programming experience was a long time ago, but my teachers would have "criticized" this kind of approach.

Reducing lines of code through optimization can be an interesting challenge, but this is not just ugly code; it is inadequate. We can all see the broken number formatting. Any experienced programmer could design a better approach than this if/else structure.

This suggests there may be no established internal review process, pair programming culture, or proper mentoring. Code like this should never make it into production-quality software. It frustrates users not because code it looks bad, but because it does not solve the task correctly: formatting numbers properly.

Well, they've actually reached it. But implemented in their own way... WinBox 4... :man_facepalming: