1

Help me to check my code, I can't get the expected output.

  • Input: prompt user to insert plain text, and key
  • Output: calculate and come out with the encrypted text by using vigenère cipher.

I can input the plain text and the key, but I can't get the cipher text output.

INCLUDE IRVINE32.INC

.DATA
    BUFMAX = 128
    sPrompt1 BYTE "Enter the plain text:", 0
    sPrompt2 BYTE "Key:",0
    sEncrypt BYTE "Cipher text:",0
    buffer BYTE BUFMAX+1 DUP(?)
    key BYTE BUFMAX+1 DUP (?)
    bufsize DWORD ?
    keysize DWORD ?

.CODE

main PROC
    call Clrscr
    call inputString            ;input plain text, key
    call translateBuffer        ;encrypt the buffer
    
    call displayMessage         ;display encrypted message

    exit
    main ENDP

inputString

inputString PROC
;--------------------------------------------------------------------------------
;Prompts user to input string and key, saves the string and it's length
;recieves: nothing
;returns: nothing
;--------------------------------------------------------------------------------
    pushad
    mov edx, OFFSET sPrompt1    ;prompt plain text
    call WriteString
    mov ecx, BUFMAX
    mov edx, OFFSET buffer
    call ReadString
    mov bufsize, eax
    call Crlf

    mov edx, OFFSET sPrompt2    ;prompt key
    call WriteString
    mov ecx, BUFMAX
    mov edx, OFFSET key
    call ReadString
    mov keysize, eax
    call Crlf

    popad
    ret
    inputString ENDP

translateBuffer

translateBuffer PROC
;--------------------------------------------------------------------------------
;translate the plain text with key to cipher text
;recieves: nothing
;returns: nothing
;--------------------------------------------------------------------------------
    pushad
    mov ecx, bufsize
    mov esi, 0
    mov edi, 0
    
L1: mov al, key[edi]
    xor buffer[esi], al
    inc esi
    inc edi
    cmp edi, keysize+1
    jne L1
    mov edi, 0
    loop L1

    popad
    ret
    translateBuffer ENDP

display encrypted message

displayMessage PROC 
;--------------------------------------------------------------------------------
; Displays the encrypted message. 
; Receives: EDX points to the message 
; Returns:  nothing 
;--------------------------------------------------------------------------------
    pushad 
    mov edx, OFFSET sEncrypt
    call WriteString 

    mov edx,OFFSET buffer 
    call WriteString 
    
    call Crlf 
    call Crlf
    popad 
    ret
    displayMessage  ENDP

END main
myleong
  • 21
  • 3
  • Sry, friends. This is my first post, not relly know about the rules. – myleong Dec 12 '20 at 13:06
  • 1
    i have inserted the expected output, input and what is the error I faced – myleong Dec 12 '20 at 13:07
  • 2
    What *does* happen when you run this. Does it print anything? That's an important part of [mcve]. Just basically saying "it doesn't work" is not helpful, especially when there's this much code. See [this](https://idownvotedbecau.se/itsnotworking/) and [this](https://idownvotedbecau.se/nodebugging/). asm questions also have a bad habit of posting a whole program, not cut down to just the part that's not working (with fixed inputs for that) so it's not very minimal either. – Peter Cordes Dec 12 '20 at 13:17
  • @PeterCordes I think this question is now sufficiently clear. I would like to answer it and have voted to reopen it. – Sep Roland Dec 12 '20 at 16:46
  • 3
    @SepRoland: Ok, since you asked. I would have left it closed until followed standard guidelines, but at least each function has comments. Especially since debugging questions usually have little to no future value for people with similar problems to be able to find them, and just clutter things up when looking for duplicates. – Peter Cordes Dec 12 '20 at 17:35
  • @PeterCordes When i am run the program, i can input plain text, and key. But for the output, only the word{Chiper text:} print only, the result after translate didn't print. – myleong Dec 13 '20 at 08:07
  • sry for posting a whole program, beacause i need other people what I am doing before. – myleong Dec 13 '20 at 08:08
  • [edit] your question to add your description. And BTW, the point of a [mcve] is *not* to just leave out parts of your code. It's to do some actual work reducing your program to a much simpler program that you can still run and still demonstrates the same problem. So a solution to that would be something you could apply in your full program. e.g. if the problem was in a strlen function, you'd want a MCVE that just calls strlen with a pointer to a static buffer (so you can see the result with a debugger), instead of prompting and then reading input and then strlen then printing the result. – Peter Cordes Dec 13 '20 at 13:57
  • But if all you know is that output isn't printing, and you haven't used a debugger to find out why, I guess you wouldn't know which parts you could simplify. That's why we use debuggers. – Peter Cordes Dec 13 '20 at 13:59

1 Answers1

1

Your input and output procedures seem fine. The troubles sit in-between, in your encryption procedure.

The user can input strings of varying lengths for the plain text and for the encryption key. Your program needs to consider these different lengths in order to not overflow any buffers, because that's exactly what is happening in your code!

Problem 1

Because the jne L1 instruction, going the wrong way, bypasses the normal loop counting, your loop runs for much too long and starts overwriting memory that does no longer belong to the buffers that were defined.
The loop must only traverse the plain text string. If the text in the encryption key happens to be shorter than the plain text string, then you will want to repeat the key. Therefore you have to reset the offset register EDI to 0, but you have to continu the loop normally (not just return to the top!).

Problem2

A second problem is that you have this cmp edi, keysize+1 instruction that, because of the +1, will be consuming the zero-terminator from the key string as if it were part of that string. That's never the idea behind a string terminator. It's not part of the string.

    push  esi                   ; No need to preserve the scratch registers EAX, ECX, EDX 
    mov   esi, OFFSET buffer
    xor   edx, edx
    mov   ecx, bufsize
L1: movzx eax, byte ptr key[edx]
    xor   [esi], al
    inc   edx
    inc   esi
    cmp   edx, keysize
    je    L3                    ; Branches in the least common case
L2: dec   ecx
    jnz   L1
    pop   esi
    ret
L3: xor   edx, edx
    jmp   L2

In the above snippet I have improved the code a bit.

  • You can zero a register simply by XORing it to itself.
  • You should never use the LOOP instruction because it is very slow.
  • And some more optimizations that you can read about in Peter Cordes' comments below this answer.

And this is a version that even eliminates the conditional branch using the CMOVE (Conditional MOVe If Equal) instruction that will store the (zeroed) EDX register over the EDI register if the Equal condition happens to be true. If not, there's no moving at all.

    xor   edi, edi
    xor   esi, esi
    xor   edx, edx
    mov   ecx, bufsize
L1: mov   al, buffer[esi]
    xor   al, key[edi]
    mov   buffer[esi], al
    inc   esi
    inc   edi
    cmp   edi, keysize
    cmove edi, edx
    dec   ecx
    jnz   L1

The danger of XOR

Because the encryption uses a mere XOR operation and depending on the composition of both strings, it's possible to obtain a zero byte amongst the bytes of the encrypted string. Obviously a print function like WriteString that depends on the zero-terminating of a string will consider the first zero as the end of the string. If it so happens that this occurs in the first position of the encrypted string, well then there's nothing at all to print...

Sep Roland
  • 33,889
  • 7
  • 43
  • 76
  • 1
    in the output, cipher text didn't show. ``` Enter the plain text:abcde Key:ab Cipher text: ``` – myleong Dec 13 '20 at 07:52
  • i have changed my code as yours, but for the output part, the cryption text didn't display – myleong Dec 13 '20 at 08:12
  • 1
    The problem is this: You have inputted a plain text that starts with a small caps 'a' and at the same time an encrytion key that also starts with a smallcaps 'a'. **The `XOR` of both produces a zero byte** that *WriteString* will consider an empty string, so there's really nothing to display! Try this: Use totally different inputs. And don't forget that if the result of any `XOR` operation produces a byte < 32, it's possible that *WriteString* will not print that either! – Sep Roland Dec 13 '20 at 15:13
  • Re: `cmove` : the downside is that it's a data dependency, so the critical path for EDI between iterations is now inc->cmp->cmov -> next iteration's inc. i.e. 3-cycle latency loop-carried dependency chain (on a CPU with single-uop / single-cycle latency `cmov`) bottlenecks you to at best 1 iteration per 3 cycles, while your loop is only 8 single-uop instructions so could otherwise run at 1 iteration per 2 cycles. Predictable branching for looping over the key would be preferable for *long* inputs, but for short inputs OoO exec can overlap this loop with code before/after, hiding latency. – Peter Cordes Dec 13 '20 at 15:58
  • 1
    For a branchy version, you'd normally want to make the common case not-taken, putting the pointer reset out-of-line so the common case is not-taken for front-end efficiency. (Conditionally jump to a block and jump back). Also, use `movzx eax, byte ptr buffer[esi]` to [avoid a false dependency](https://stackoverflow.com/questions/41573502/why-doesnt-gcc-use-partial-registers) on last iteration's EAX; `mov al, mem` merges a byte on CPUs after Sandybridge (and non-Intel), decoding as a micro-fused load + ALU merge. Byte xor and store are still fine; xor already reads its destination. – Peter Cordes Dec 13 '20 at 16:03
  • Of course if you actually cared about performance, obviously you'd go a dword or xmm at a time when far enough from the end of the key to go about 4x or 16x faster. And of course a pointer increment would save code size, esp. for the `buffer` being used twice. If you don't use XMM, you could load the key and them memory-destination XOR it. That shrinks the code-size but doesn't save any front-end bandwidth (except on Zen where a 2 uop instruction can help it max out the 5 instruction / 6 uop issue/rename width) – Peter Cordes Dec 13 '20 at 16:08
  • 1
    Oh lol, just looked at this again. If you cared about performance, you wouldn't use `pushad` / `popad` either. Looks like EDX is unused, so you only need to save/restore one call-preserved register to have enough. – Peter Cordes Dec 13 '20 at 16:27
  • @PeterCordes Does it matter the order in which the pointers get incremented? `inc esi` `inc edi` `cmp edi, keysize` or else `inc edi` `inc esi` `cmp edi, keysize` ? – Sep Roland Dec 13 '20 at 16:30
  • 1
    Not really, OoO exec will hide things. But generally putting the `inc edi` farther ahead of the `cmp` might slightly help. Chances are all 3 instructions will issue into the back-end in the same clock cycle anyway, but there are older in-order Atoms. Oh, `keysize` isn't an assemble-time constant? Typically you'd want to load it into a register, but I think `cmp reg, [mem]` / `je` can still micro- and macro-fuse into a single compare-and-branch+load uop for the front-end. And I guess you'd have enough load-port throughput on SnB-family to do 3 loads + 1 store per 2 clocks. – Peter Cordes Dec 13 '20 at 16:44
  • Oh, I hadn't noticed that you commented in your answer about changing the OP's memory dst xor. That was good optimization advice for P5 Pentium which couldn't decompose instructions into their component uops. (And somewhat for CPUs without a uop cache (P6 family), where decode of a multi-uop instruction that needs the "complex" decoder happens every time through the loop). These days a memory-destination add/xor/whatever is typically better than separate load/ALU/store, except for `adc` because of TLB coherency, apparently. Here it's a wash because it's still ALU+load for the other operand – Peter Cordes Dec 13 '20 at 16:52
  • Related to my first comment: [Avoid stalling pipeline by calculating conditional early](https://stackoverflow.com/q/49932119) – Peter Cordes Dec 13 '20 at 16:52
  • Note that on early P6 CPUs, before they could do micro-fusion (and still for indexed addressing modes on CPUs before Haswell), a `mov` store was 2 uops for the front-end not just the back end, and thus needed the "complex" (first) decoder. So depending on uop counts in surrounding instructions, on PIII it could be good to get 4 uops from `xor [mem], al`, plus decode the next 3 single-uop instructions in the same cycle. (Before SnB, Core 2 decoders could handle 4-1-1-1 = 7 uops. SnB it's a max of 4, like 1-1-1-1 or 2-1-1 or 3-1 or 4). i.e. if you need a multi-uop insn, maybe make it bigger – Peter Cordes Dec 13 '20 at 16:57
  • But Core 2 could micro-fuse a mov store so breaking it up like you have here could be good for Core 2's decoders. Like I said, these days you typically want a memory destination instruction unless it's `adc`, if an RMW is what you really want. – Peter Cordes Dec 13 '20 at 16:59
  • Sry for late reply, i am a newbie for this course, I only learnt 2 month on this assembly, then need to do assignment already. – myleong Dec 15 '20 at 01:59
  • Enter the plain text:ABCDE Key:123 Cipher text:pppDOpp – myleong Dec 15 '20 at 01:59
  • this is the output, why cipher text > plain text – myleong Dec 15 '20 at 02:00
  • Enter the plain text:abcdefghij Key:1 Cipher text:PbiPbiPbiPbi – myleong Dec 15 '20 at 02:05
  • The conversion from "ABCD" into "pppD" proves that **you are not resetting the index for accessing the key**. Make sure that you use the first code snippet in my answer (the one that uses the 3 labels *L1*, *L2*, and *L3*) I guess that you are, just like you did before in your original program, jumping to the wrong places! Please look very, very close at the names of these labels. – Sep Roland Dec 15 '20 at 19:17
  • @myleong If need be then post your updated program as an edit to your current post. Below everything the post contains already, you write the word "[edit]" followed by the complete new version. Copy/Paste it, don't make last minute changes! – Sep Roland Dec 15 '20 at 19:24