8 Replies - 421 Views - Last Post: 20 January 2019 - 03:34 PM Rate Topic: -----

#1 sheshach   User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 14
  • Joined: 31-December 18

When "-Wall" is short for wall of warnings

Posted 18 January 2019 - 10:23 PM

Seriously I don't even know how my code compiles and runs with this many warnings. I've checked it out with valgrind to make sure it's not a memory leaking mess as well, and it's all good. Except when I run gcc with '-Wall", and I get about a thousand -Wpointer-sign warnings. I tried to just get busy fixing them, but then there's stuff like this...

src/passmanager.c: In function ‘genPassWord’:
src/passmanager.c:2978:12: warning: pointer targets in passing argument 1 of ‘strcpy’ differ in signedness [-Wpointer-sign]
     strcpy(entryPass, tempPassString);
            ^
In file included from src/passmanager.c:34:0:
/usr/include/string.h:125:14: note: expected ‘char * restrict’ but argument is of type ‘unsigned char *’
 extern char *strcpy (char *__restrict __dest, const char *__restrict __src)


What the heck is "char *__restrict" and should I typecast to that, or should I set 'entryPass' to be that type? Meanwhile what about every other thing that uses 'entryPass' that wants either 'char' or 'unsigned char' but not this weird "char *__restrict". And from what I've read, typecasting is generally discouraged for this predicament anyway.

I keep hearing the mantra that no warnings should ever be ignored, but to fix all of these warnings feels like I'm practically rewriting the application. So does that mean it NEEDS to be re-written? What circumstances justify just putting up with "bad code"?

A part of me feels like ignorance is bliss and I should have never turned on -Wall. I mean seriously the program runs perfectly, valgrind checks out fine with it, etc. Am I trying to fix something that's not broken?

Brief reading tells me this could affect portability, but I don't fully comprehend why. Something about 2's compliment and some bit representations for small numbers being the same for large numbers. However, what if I only anticipated it to be used with one OS, and maybe even just one CPU architecture?

I was just curious what warnings would show up. Curiosity kills the cat. I was feeling pretty encouraged about getting back into C until now.

Huge pastebin of warnings:
https://pastebin.com/LWYfBKzZ
Huge crummy program they came from:
https://pastebin.com/dFMrpH1A
(link with -lcrypto and -lcap)

P.S.
I think I'm going to be developing with -Werrors on from now on so I don't write an entire application full of warnings and get stuck trying to fix them at the end. *facepalms*

Is This A Good Question/Topic? 1
  • +

Replies To: When "-Wall" is short for wall of warnings

#2 Salem_c   User is online

  • void main'ers are DOOMED
  • member icon

Reputation: 2295
  • View blog
  • Posts: 4,397
  • Joined: 30-May 10

Re: When "-Wall" is short for wall of warnings

Posted 18 January 2019 - 11:56 PM

Well the warnings arise because you used 'unsigned char' rather than 'char' for all your strings.

The restrict is a type qualifier -> https://en.cpprefere...nguage/restrict
Was This Post Helpful? 0
  • +
  • -

#3 sheshach   User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 14
  • Joined: 31-December 18

Re: When "-Wall" is short for wall of warnings

Posted 19 January 2019 - 03:17 AM

View PostSalem_c, on 18 January 2019 - 11:56 PM, said:

Well the warnings arise because you used 'unsigned char' rather than 'char' for all your strings.

The restrict is a type qualifier -> https://en.cpprefere...nguage/restrict


Oh, yeah, that was more of my bone-headedness thinking they all needed to be the same, so I was trying switching between making every instance of a 'char' unsigned, or vise versa. But say I changed 'entryPass' to a 'char', that will satisfy strcpy, but then I have another problem...

src/passmanager.c: In function ‘allocateBuffers’:
src/passmanager.c:2844:21: warning: pointer targets in passing argument 1 of ‘RAND_bytes’ differ in signedness [-Wpointer-sign]
     if (!RAND_bytes(entryPass, BUFFER_SIZES)) {
                     ^
In file included from src/passmanager.c:29:0:
/usr/include/openssl/rand.h:101:5: note: expected ‘unsigned char *’ but argument is of type ‘char *’
 int RAND_bytes(unsigned char *buf, int num);
     ^


I allocate 512 bytes to entryPass in a buffer, and I then insert a null-terminated string into that as the password, and pad the brest of the buffer. Terminal output uses only string functions so the null terminator prevents padding from being printed, but I write all of it out in binary to the data file. OpenSSL's CSPRNG wants it to be 'unsigned char', but all the string functions want 'char'.

The same conflict exists for basically all of the buffers because I'm padding all of the strings which take user input. If I were to make a separate string array to hold the input, and then copy that into the padded buffer it might work... But then wouldn't I just be copying from an 'unsigned char' buffer into a 'char' buffer and have the same problem? Plus I'd have to figure out how to make sure there's enough room for the input, which having the padded buffer was helping to address.

In a situation like that, shouldn't I keep it as an 'unsigned char' for OpenSSL's CSPRNG, and but then typecast it for functions that need it as a 'char'? I thought I remembered something about typecasting only working from larger to small.
Was This Post Helpful? 0
  • +
  • -

#4 Salem_c   User is online

  • void main'ers are DOOMED
  • member icon

Reputation: 2295
  • View blog
  • Posts: 4,397
  • Joined: 30-May 10

Re: When "-Wall" is short for wall of warnings

Posted 19 January 2019 - 03:48 AM

> But then wouldn't I just be copying from an 'unsigned char' buffer into a 'char' buffer and have the same problem?
Not if you use memmove() or memcpy()

Anything which is printable, has a \0 at the end and you want to treat as a string -> char
Anything crypto (hashes, salts, PRNG, etc) which contains binary data -> unsigned char

The distinction might help prevent you from doing something silly with the wrong kind of buffer.

It might be a bit annoying, but you will have a nice clean line between the cleartext world and the crypto world in your program.
Was This Post Helpful? 1
  • +
  • -

#5 sheshach   User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 14
  • Joined: 31-December 18

Re: When "-Wall" is short for wall of warnings

Posted 19 January 2019 - 11:14 PM

Oh okay sorting them according to if they're strings or crypto buffers makes a lot of sense.

I got started on doing that, but I noticed some functions want a "const char *" and that also confuses me a bit. If I declare a buffer to be const, how can I ever put a password into it, and then send it to the function?


I'm also a little bit confused about your suggestion to use memcpy and memmove. If I had something like...

char inputStringHolder[513]; /*Because I want to be able to contain strings up to 512 characters long*/
unsigned char *paddedTmpBuffer;

/*Fill paddedTmpBuffer with CSPRNG bytes*/

/*Get some string into inputStringHolder and make sure it's not too big*/

memcpy(paddedTmpBuffer,inputStringHolder, strlen(inputStringHolder));

/*Copy the bytes from paddedTmpBuffer into the buffer it's intended for*/




That would copy everything up to the '\0' into the paddedBuffer, but wouldn't it still be copying from a signed source into an unsigned destination? Or does memcpy do something in its inner works that makes that not matter?


What types of situations is simply typecasting appropriate for?
Was This Post Helpful? 0
  • +
  • -

#6 Salem_c   User is online

  • void main'ers are DOOMED
  • member icon

Reputation: 2295
  • View blog
  • Posts: 4,397
  • Joined: 30-May 10

Re: When "-Wall" is short for wall of warnings

Posted 20 January 2019 - 04:58 AM

Well memmove/memcpy have the advantage that their pointer parameters are void*, which means you can point at anything you like.

Whilst memcpy(&myInt,&myFloat,sizeof(myInt); might be syntactically valid, it's semantically meaningless. It's up to you to supply the semantic meaning of whatever you're copying.
Was This Post Helpful? 1
  • +
  • -

#7 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 6717
  • View blog
  • Posts: 22,931
  • Joined: 05-May 12

Re: When "-Wall" is short for wall of warnings

Posted 20 January 2019 - 06:08 AM

View Postsheshach, on 20 January 2019 - 01:14 AM, said:

I got started on doing that, but I noticed some functions want a "const char *" and that also confuses me a bit. If I declare a buffer to be const, how can I ever put a password into it, and then send it to the function?

The const is an indicator that the function will not modify that parameter. It does not mean that you have to pass in a const value there.

For example, notice in strcpy() that the first parameter is not const, but the second one is. So this let's you have the following code to be legal:
char src[100];
char dst[100];

strcpy(src, "Hello World!");
strcpy(dst, src);


Was This Post Helpful? 1
  • +
  • -

#8 sheshach   User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 14
  • Joined: 31-December 18

Re: When "-Wall" is short for wall of warnings

Posted 20 January 2019 - 02:16 PM

Okay that definitely un-complicates things a lot! I couldn't figure out how I was supposed to give it a const.

Salem_c, so what about an example like this... (Which when tested, worked fine)

void allocateBuffers()
{
	unsigned char *tmpBuffer = calloc(sizeof(unsigned char), BUFFER_SIZES);
	
    entryPass = calloc(sizeof(char), BUFFER_SIZES);
    if (!RAND_bytes(tmpBuffer, BUFFER_SIZES)) {
        printf("Failure: CSPRNG bytes could not be made unpredictable\n");
        exit(1);
    }
    memcpy(entryPass,tmpBuffer,sizeof(unsigned char) * BUFFER_SIZES);
}


Versus just typecasting with this... (Also worked fine)

void allocateBuffers()
{
    entryPass = calloc(sizeof(char), BUFFER_SIZES);
    if (!RAND_bytes((unsigned char *)entryPass, BUFFER_SIZES)) {
        printf("Failure: CSPRNG bytes could not be made unpredictable\n");
        exit(1);
    }
}



Would there be any real benefit to adding more memcpy functions since I'd have several other buffers to do this for? I was reading memcpy's manual and it has some ominous warnings about memory over-lapping that make me wonder if managing a bunch of memcpy's could be more perilous than just typecasting what RAND_bytes is seeing?

I know that it shouldn't matter for my functions which will input data into this buffer, because they will just over-write any 'unsigned char' bytes with the 'char' bytes.

But later on, I must copy that back into an 'unsigned char' buffer for the encryption routines.

    /*entryPass and entryName are both copied into infoBuffer, which is then encrypted*/
    unsigned char* infoBuffer = calloc(sizeof(unsigned char), BUFFER_SIZES * 2);
    unsigned char* decryptedBuffer = calloc(sizeof(unsigned char), fileSize + (BUFFER_SIZES * 2) + EVP_MAX_BLOCK_LENGTH);
    unsigned char* encryptedBuffer = calloc(sizeof(unsigned char), fileSize + EVP_MAX_BLOCK_LENGTH);

    /*Put the chars, include random whitespace ones, from entryName and entryPass into infoBuffer, again splitting the BUFFER_SIZES * 2 chars between the two*/
    for (i = 0; i < BUFFER_SIZES; i++)
        infoBuffer[i] = entryName[i];
    for (i = 0; i < BUFFER_SIZES; i++)
        infoBuffer[i + BUFFER_SIZES] = entryPass[i];


So with the for loops ( know there's probably a more graceful way to do this ) I first copy 512 bytes of the entryName into the infoBuffer, then I copy 512 bytes of the entryPass into the infoBuffer, and then the infoBuffer is what's given to the EVP functions which want 'unsigned char'.

I'm just guessing... But since the user-input bytes are 'char' from the string functions, once the whole buffer is passed to the encryption function as 'unsigned char', those 'char' bytes will be converted, but just by having extra bits tagged on? Then because printing those 'unsigned char' bits as 'char' simply lops those added bits back off, it doesn't corrupt what the original char byte was from user-input? I hope what I just said made any sense.

Well if it did make sense, and I'm correct, then wouldn't the conversion of those 'unsigned char' bits from RAND_bytes into 'char' bytes be negligible and make the typecast adequate to avoid the warnings from the compiler, or should I use the memcpy form?

Anyway I feel like I'm confusing myself a little now.
Was This Post Helpful? 0
  • +
  • -

#9 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 6717
  • View blog
  • Posts: 22,931
  • Joined: 05-May 12

Re: When "-Wall" is short for wall of warnings

Posted 20 January 2019 - 03:34 PM

The memory overlapping issue is only when you trying to move bytes around within the same block of array. In your example above where you are simply copying from one buffer to another buffer, won't be any condition where you will have overlapping source and destinations.

Regardless of using memcpy() or generating random bytes directly into entryPass, here is the problem. You are sticking unsigned bytes into a buffer that you are saying contains signed bytes. This is one example of mixing cipher text and key text.

entryPass is supposed to be a C string that can be displayed to the user (based on my quick scan through your gist). User displayable C strings normally only store ASCII characters. ASCII characters range only from 0-127. So what happens when your randomizer generates the a value of 240? What ASCII character is that supposed to be?

Even if you say that, you are taking advantage of UTF-8 strings and so the full signed range from -128 to 127, how are you going to unsure that RAND_bytes() will generate valid UTF-8 values?
Attached Image

And lastly, let's say that you entryPass is BUFFER_SIZES bytes long. How do you ensure that there won't be a null terminator in the middle of the buffer?

Yes, I know that you get around these problems that I just cited in your genPassword() function. And you try to make sure that every code path calls genPassword() if you weren't given a specific password to use. But this simply trying to ensure this kind of code path doesn't scale, especially if you have multiple developers or if you spend a lot of time away from the code and then jump back into it. It'll be very easy to make a mistake. As noted by Salem_c, in the crypto world, one way to avoid making mistakes like this is to ensure that you draw nice clear lines between clear text and cipher text. One team I worked with had a naming convention for which made it crystal clear what kind of data you were dealing with based on the variable names, as well as by ensuring that they were kept in separate data structures. Any code which straddled the two worlds had a specific naming convention to remind code reviewers to be extra vigilant.
Was This Post Helpful? 1
  • +
  • -

Page 1 of 1