9

I have decided to create a simple guessing number game that uses Linux system calls, and some C functions to provide a more simpler interface. I seem to get a segmentation fault when I convert the int to string and print the correct answeron the screen.

Here is the output:

Enter A Number One Through Ten:" : 
3
Response did not match! The Answer Is:Segmentation fault

Here is the C code:

// print.c
#include "/usr/include/stdio.h" 
#include "/usr/include/string.h"
#include "/usr/include/stdlib.h"
#include "/usr/include/time.h"
void print(const char* msg)
{
    printf(msg);
    return;
}
int compare(const char* str, const char* str2)
{
    int i = strcmp(str, str2);
    if (i == 0)
    {
        return 1;
    }
    else
    {
       return 0;
    }
}
int divide(int num, int dem)
{
    if (dem == 0)
    {
        printf("Undefined");
        return 0;
    }
    else {
        return (num / dem);
    }
}
int randnum(int maxn)
{

    if (maxn == 0)
    {
        maxn = 1;
    }
    srand(time(0));
    return rand() % maxn;
}
int stoi(const char* str)
{
    return atoi("str");
}
void itos(int n)
{

     char* buf = "5";
     int ret = sprintf(buf, "%i\n", n);
     if (ret == -1){
    printf("Error!");
    return;
     }
     else{
    printf(buf);
     }
     return;

}

Here is the NASM Code:

      ; Declared C functions.
        extern print 
        extern compare
        extern divide
        extern randnum
        extern stoi
        extern itos
        section .data 
            msg: db 'Enter A Number One Through Ten:" : ', 10
            ml: equ $ - msg
            t: db 'Response did match!', 10
            tl: equ $ - t
            f: db 'Response did not match! The Answer Is:', 0
            fl: equ $ - f
            str2: db 'Hello'
        section .bss
            ;srnum: resb 255
            snum: resb 255
            rnum: resb 255
            num: resb 255
        section .text
            global _start ; Entry point function or label.
        _start:
            ; System call sys_write
            mov eax, 4
            mov ebx, 1
            mov ecx, msg
            mov edx, ml
            int 80h

        ; System call sys_read
        mov eax, 3
        mov ebx, 0
        mov ecx, snum
        mov edx, 255
        int 80h

        ; Call stoi which converts string to int (parameter 1: is string to convert).
        push snum
        call stoi
        mov [num], eax

        mov ecx, esp
        sub ecx, 4
        mov esp, ecx


        ; Call random
        push 10
        call randnum
        mov [rnum], eax


        mov ecx, esp
        sub ecx, 4
        mov esp, ecx

        ; Compare the two integers.
        mov eax, num
        cmp eax, [rnum]
        je true
        jne false

    true:
        ; Call sys_write 
        mov eax, 4
        mov ebx, 1
        mov ecx, t
        mov edx, tl
        int 80h

    false: ; Segmentation fault is somewhere in this label 

        mov eax, 4
        mov ebx, 1
        mov ecx, f
        mov edx, fl
        int 80h


        push rnum
        call itos 


        ; Calling sys_exit with exit code (0 = ERROR_SUCCESS)
        mov eax, 1
        mov ebx, 0
        int 80h

osgx
  • 90,338
  • 53
  • 357
  • 513
Daniel Lopez
  • 177
  • 1
  • 1
  • 5
  • 3
    +1 just for the title :) – Earlz Feb 17 '11 at 01:30
  • 1
    Okay, I'm going to resist my first inclination (which is to ask "Why? Seriously, why?" :-) I think you have the right answer but I'm curious as to why the `f` message is null terminated. That's not required for syscall 4. Perhaps it's left over from a cut and paste? – paxdiablo Feb 17 '11 at 02:07
  • 1
    Daniel, you can use `#include ` instead of `#include "/usr/include/header.h"` for system headers (unless there is some path configuration problem?) – Fernando Feb 17 '11 at 03:00
  • 1
    It's not the cause of your crash, but your stoi function won't work. You need to call atoi(str), not atoi("str").. what you have will try to convert literal "str" to a number. – Splat Feb 17 '11 at 03:33
  • 2
    Gotta love the "The answer is: Segmentation fault" You are like YES ITS GONNA WORK, ohhhh . – ChemiCalChems Apr 02 '15 at 18:03

3 Answers3

6

There is a problem with this code:

char* buf = "5";
int ret = sprintf(buf, "%i\n", n);

buf is a pointer to readonly memory, and sprintf wants to be able to modify its contents. You should change buf to an array: char buf[20] (or some number other than 20 that is arbitrarily large enough to hold your desired contents)

Marlon
  • 19,924
  • 12
  • 70
  • 101
  • Whether string literals are in read-only memory is language (C/C++) and implementation defined...Not saying you're wrong in this case. The memory allocated for "5" is not enough to hold the output of that sprintf anyway, but that kind of buffer overflow won't necessarily cause an immediate crash (only later :() – Splat Feb 17 '11 at 03:26
  • @Splat: Nevertheless, [it's UB in ISO C to write to a string literal](https://stackoverflow.com/questions/4493139/are-string-literals-const/18704903#18704903), despite the fact that its type isn't explicitly `const`. Also, this is an assembly question, so we happen to know that it's GCC targeting i386 Linux, and thus yes the strings are in read-only memory unless you compiled with `-fwritable-strings` (in clang or an old gcc version from before that option was removed). In that case, the compiler *does* define that behaviour which ISO C left undefined. (And presumably disables deduplication) – Peter Cordes Jun 11 '19 at 00:19
5
void itos(int n)
{

     char* buf = "5";

In buf you have space for 2 chars (the five and \0)

But here:

int ret = sprintf(buf, "%i\n", n);

you insert in it at least 3 chars, at least one digit of the number, the break line \n, and then \0.

Also is incorrect to modify a literal string this way. You can declare a stack copy of a literal string in the next way:

char buf[] = "5"; // This sample will fail anyway, use a larger string...

Or better just an empty array big enougth for several digits:

char buf[1024];
Fernando
  • 1,382
  • 8
  • 17
  • `char buf[] = "5"` will allocate an array of 2 bytes, `sprintf("%i\n" ...)` will cause a buffer overflow. Go with the latter example :P – Marlon Feb 17 '11 at 02:02
  • @Marlon `char buf[] = "nah, just give a little :-)"` –  Feb 17 '11 at 02:06
  • 1
    Yes the first example stills a bad idea as @Marlon says because of the reserved space, and also because it is not necessary any initialization in this problem :) – Fernando Feb 17 '11 at 02:08
3

In your function itos(), you are attempting to modify the string literal "5". String literals are non-modifiable (in this case, your OS stores them in memory mapped as read-only).

In this case, your itos() function is needlessly complicated - you could simply replace it with:

void itos(int n)
{
     printf("%i\n", n);
}

(..or you could just directly call printf() from your asm code).

caf
  • 233,326
  • 40
  • 323
  • 462
  • Thanks a lot, I should have looked at the warnings because what you said about ROM strings was actually being foretold by my compiler. Usually I discard warnings but hey you learn a new lesson everyday. I guess my problem was much simpler than I expected. – Daniel Lopez Feb 17 '11 at 02:10
  • Never mind it didn't warn me about that, it warned me about something else. Thanks any ways, it works now. – Daniel Lopez Feb 17 '11 at 02:21
  • 1
    You should never ever ignore warnings. From looking at your code, you seem to be a beginner, in which case that statement is doubly true. –  Feb 22 '11 at 21:00