c++ reading argv into unsigned char fixed size: Segmentation fault

0
votes

I am trying to read command line argument into a fixed size unsigned char array. I get segmentation fault.

My code:

#include <stdio.h>
#include <iostream>
#include <stdlib.h>
#include <memory.h>

unsigned char key[16]={};

int main(int argc, char** argv){
        std::cout << "Hello!" << std::endl;
        long a = atol(argv[1]);
        std::cout << a << std::endl;
        memcpy(key, (unsigned char*) a, sizeof key);
//      std::cout << sizeof key << std::endl;
//      for (int i = 0; i < 16; i++)
//              std::cout << (int) (key[i]) << std::endl;
        return 0;
}

What am I doing wrong?

To call the program:

compile: g++ main.cpp

Execute: ./a.out 128

2
c++segmentation-faultargvunsigned-char
your question is incomplete. How do you call your program? - Jean-François Fabre
what did you pass to main???? - Raindrop7
It can be any number from 0 to 2^128, right?!! - algoProg
@algoProg The right tool to solve such problems is your debugger. You should step through your code line-by-line before asking on Stack Overflow. For more help, please read How to debug small programs (by Eric Lippert). At a minimum, you should [edit] your question to include a Minimal, Complete, and Verifiable example that reproduces your problem, along with the observations you made in the debugger. - πάντα ῥεῖ
For what it's worth, never use atol, because there is another standard function, strtol that serves the same purpose but with much better error handling. - Ben Voigt

2 Answers

2
votes

You get SEGV because your address is wrong: you convert a value to an address. Plus the size is the one of the destination, should be the size of the source

The compiler issues a warning, that's never good, you should take it into account because that was exactly your error:

xxx.c:12:38: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

     memcpy(key, (unsigned char*) a, sizeof key);
                                  ^

fix that like this:

memcpy(key, &a, sizeof(a));

BTW you don't have to declare key with 16 bytes. It would be safer to allocate it like this:

unsigned char key[sizeof(long)];

and when you print the bytes, iterate until sizeof(long) too, or you'll just print trash bytes in the end.

Here's a fix proposal using uint64_t (unsigned 64-bit integer from stdint.h which gives exact control on the size), zero initialization for your key and parsing using strtoll:

#include <stdio.h>
#include <iostream>
#include <stdlib.h>
#include <memory.h>
#include <stdint.h>

unsigned char key[sizeof(uint64_t)]={0};

int main(int argc, char** argv){
        std::cout << "Hello!" << std::endl;
        uint64_t a = strtoll(argv[1],NULL,10);
        memcpy(key, &a, sizeof a);

      for (int i = 0; i < sizeof(key); i++)
              std::cout << (int) (key[i]) << std::endl;
        return 0;
}

(if you want to handle signed, just change to int64_t)

Test on a little endian architecture:

% a 10000000000000
Hello!
0
160
114
78
24
9
0
0
0
votes

Looks like you are copying too much data. I also added a &a for the memcpy.

#include <stdio.h>
#include <iostream>
#include <stdlib.h>
#include <memory.h>

unsigned char key[16]={};

int main(int argc, char** argv)
{
   memset(key,0x0, sizeof(key));
   std::cout << "Hello!" << std::endl;
   long a = atol(argv[1]);
   std::cout << a << std::endl;

   // the size parameter needs to be the size of a
   // or the lesser of the size of key and a
   memcpy(key,(void *) &a, sizeof(a));
   std::cout << "size of key " << sizeof(key) << "\n";
   std::cout << "key " << key << "\n";
   for (int i = 0; i < 16; i++)
   std::cout << "   " << i << "    '"  << ((int) key[i]) << "'\n";
   return 0;
}