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

Question

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

@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)](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). At a minimum, you should \[edit] your question to include a [Minimal, Complete, and Verifiable](http://stackoverflow.com/help/mcve) example that reproduces your problem, along with the observations you made in the debugger. — πάντα ῥεῖ, Sep 22 '16 at 20:03
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, Sep 22 '16 at 20:08

Jean-François Fabre · Accepted Answer · 2016-09-22T21:41:48.437

2

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

edited Sep 22 '16 at 21:41

answered Sep 22 '16 at 19:52

Jean-François Fabre

137,073
23
153
219

Hi Jean, that's my problem. Shall I make it long long? – algoProg Sep 22 '16 at 19:52
@1201ProgramAlarm: not exactly, you're were right, thanks. I was missing the convertion from integer to address! – Jean-François Fabre Sep 22 '16 at 19:55
That fixed it. No more fault. How can I check what value is in key now? My commented print loop does not work. Compiler prints '?'. – algoProg Sep 22 '16 at 19:59
Hi, my key should be 16 bytes. So if a number is less than 2^128 then it should be added in the front with 0's. That's why I have it like that. – algoProg Sep 22 '16 at 20:00
good luck with 2^128 in a `long`. Even `unsigned long long` usually ranges up to 2^64-1 max – Jean-François Fabre Sep 22 '16 at 21:12
Hi Jean, thanks for all efforts! Appreciated! Also yeah, about the long size!! – algoProg Sep 22 '16 at 21:36
no problem. I learned some new tricks too (`strtoll`) with other people's comments. – Jean-François Fabre Sep 22 '16 at 21:37
Suggest using fixed-width types of the intended width (e.g. `uint64_t`) – M.M Sep 22 '16 at 21:40
@M.M I always forget that. Edited. – Jean-François Fabre Sep 22 '16 at 21:42

score 0 · Answer 2 · answered Sep 22 '16 at 20:25

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;
}

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

2 Answers2