tags:

views:

93

answers:

5

Hi,

I didn't used C for a lot of time, and now I have to modify a little piece of code. There one thing I can't understand:

char filename[20];
filename[0] = '\0';
for (j=0; j < SHA_DIGEST_LENGTH; j++){
  sprintf(filename + strlen(filename),"%02x",result[j]);
}

In the first line a string of 20 characters is dleclared. In the second line the first char is set to '\0', so is an empty string, I suppose.

In the for loop I don't understand the "sum" between filename and its length... The firs parameter of sprintf should be a buffer where to copy the formatted string on the right. What is the result of that sum? It seems to me like I'm trying to sum an array and an integer...

What I'm missing?

+13  A: 

It's pointer arithmetic. strlen returns the number of characters before the NUL terminator. The result of the addition will point to this terminator. E.g. if the current string is "AA" (followed by a NUL), strlen is 2. filename + 2 points to the NUL. It will write the next hex characters (e.g. BB) over the NUL and the next character. It will then NUL-terminate it again (at filename + 4). So then you'll have "AABB" (then NUL).

It doesn't really make sense though. It wastes a lot of time looking for those NULs. Specifically, it's a quadratic algorithm. The first time, it examines 1 character, then 3, 5, 7, ..., 2 * SHA_DIGEST_LENGTH - 1) that . It could just be:

sprintf(filename + 2 * j,"%02x",result[j]);

There's another problem. A hexadecimal representation of a SHA-1 sum takes 40 characters, since a byte requires two characters. Then, you have a final NUL terminator, so there should be 41. Otherwise, there's a buffer overflow.

Matthew Flaschen
If you do that, don't forget to null-terminate it manually.
zebediah49
@zebediah, what do you mean? sprintf will always add a NUL terminator.
Matthew Flaschen
`It will write the next hex character (e.g. C) over this NUL, then NUL-termiante it again ` Does `sprintf` add a `\0` by default?
Amarghosh
Oops... [yes, it does](http://www.cplusplus.com/reference/clibrary/cstdio/sprintf/)
Amarghosh
+1 for pointing out inefficient algorithm and array length bug. And for explaining the pointer arithmetic :)
Mark Pim
I can't give a +1 becouse of my reputation (seems like I'm a no good!) but it's a very good answer!
Segolas
+1 for same reasons Mark Pim gave.
R..
A: 

you are adding strlen(filename) only to do concatenation of result[j]

Each iteration concatenates the current result[j] at the end of filename so each time you need to know to offset within the filename where the concatenation should take place.

Daniel Băluţă
+1  A: 

First iteration, when j = 0, you will write 3 chars (yes, including the '\0' terminating the string) onto the beginning of filename, since strlen() then returns 0. Next round, strlen() returns 2, and it will continue writing after the first two chars.

Be careful for stepping outside the 20 char space allocated. Common mistake is to forget the space required for the string terminator.

EDIT: make sure that SHA_DIGEST_LENGTH is not greater than 9.

MattBianco
+1  A: 

Why dont you declare

char filename[SHA_DIGEST_LEN*2 +1]; /* And +1 if you want to have the NULL terminating char*/

This is because SHA1 digest length is 20 bytes, if you were just to print the digest then you may probably not want the additional memory but since you want hexadecimal string of the digest you can use the above declaration. A strlen operation returns lenghth of string till a null terminating character is encountered.

So basically when you do the following :

sprintf(filename + strlen(filename),"%02x",result[j]);

In the first interation filname is copied with 2 bytes of the hexadecimal representation of the first byte of the sha-1 digest. Eg. Say that is AA, now you need to move your pointer two places to copy the next byte.

After second iteration it becomes AABB. After the 20th iteration you have the entire string AABBCC......AA[40 bytes] and +1 if you need the '\0' which is the NULL termination character.

Praveen S
A: 

Replace the code with:

char filename[SHA_DIGEST_LENGTH*2+1];
for (j=0; j < SHA_DIGEST_LENGTH; j++){
  sprintf(filename + 2*j,"%02x",result[j]);
}

Faster, simpler, and the bugs are gone.

R..