Building up a string in C. Am I doing it right?

Question

A while back I asked this question.

I eventually hacked together a sort of solution:

int convertWindowsSIDToString(void *sidToConvert, int size, char* result) {
    const char *sidStringPrefix = "S-";
    int i;
    int concatLength = 0;
    /* For Linux I have SID defined in a seperate header */
    SID *sid;   
    char revision[2], identifierAuthority[2];

    if(sidToConvert == NULL) {
        return 1;
    }

    sid = (SID *)sidToConvert;  

    snprintf(revision, 2, "%d", sid -> Revision);
    snprintf(identifierAuthority, 2, "%d", sid -> IdentifierAuthority.Value[5]);

    /* Push prefix in to result buffer */
    strcpy (result,sidStringPrefix);
    /* Add revision so now should be S-{revision} */
    strcat(result, revision);
    /* Append another - symbol */
    strcat(result, "-");
    /* Add the identifier authority */
    strcat(result, identifierAuthority);


    /* Sub Authorities are all stored as unsigned long so a little conversion is required */
    for (i = 0; i < sid -> SubAuthorityCount; i++) {
        if(concatLength > 0){
            concatLength += snprintf(result + concatLength, size, "-%lu", sid -> SubAuthority[i]);
        } else {
            concatLength = snprintf(result, size, "%s-%lu", result, sid -> SubAuthority[i]);
        }
    }

    return 0;
}

I'm a complete amateur at C. In the few test cases I have run, this works fine but I am worried about how I'm handling strings here.

Is there any better way to handle string concatenation in this type of scenario? Please note, I am kind of tied to C89 compatibility as I am trying to keep all code compiling on all platforms and am stuck with Visual Studio on Windows for now.

Also my apologies if this question is not the best format for Stack Overflow. I guess I'm asking more for a code review that a very specific question but not sure where else to go.

EDIT

Just wanted to add what I think is almost the final solution, based on suggestions here, before accepting an answer.

int convertWindowsSIDToString(SID *sidToConvert, int size, char* result) {
    int i;  
    char* t;    
    if(sidToConvert == NULL) {
        printf("Error: SID to convert is null.\n");
        return 1;
    }   
    if(size < 32) {
        printf("Error: Buffer size must be at least 32.\n");
        return 1;
    }
    t = result;
    t+= snprintf(t,  size, "S-%d-%d", sidToConvert->Revision, sidToConvert->IdentifierAuthority.Value[5]);

    for (i = 0; i < sidToConvert -> SubAuthorityCount; i++) {
        t += snprintf(t, size - strlen(result), "-%lu", sidToConvert -> SubAuthority[i]);
    }

    return 0;
}

I've got a lot of reading to do yet by the look of things. Got to admit, C is pretty fun though.

(almost) always use the `n` functions for security - `snprinf`, `strncpy`, `strncat`... — fotanus, Jan 23 '14 at 11:57
`sizeof result` is the size of a pointer. If your tests work with this code, then you're been working with strings shorter than 8 characters. Since you can't know the size of an allocation from a pointer alone in C, the check is useless. — Fred Foo, Jan 23 '14 at 11:58
@Iarsmans - You're right. I shouldn't have added that in as it was something I was trying out but hadn't tested before posting here. Editing sample code. — Ruairi O'Brien, Jan 23 '14 at 12:00
@larsmans only partly true. `char ptr[512]; sizeof(ptr)` will give you 512. But it is true for `char *` pointers. — ckruse, Jan 23 '14 at 12:01
Now you have a useless `strcpy`: just building the string in `result` directly would skip one potential buffer overflow. — Fred Foo, Jan 23 '14 at 12:02
@fotanu I agree with you. Some issues in Visual Studio like here: http://stackoverflow.com/questions/2915672/snprintf-and-visual-studio-2010. I will try to work around them though. — Ruairi O'Brien, Jan 23 '14 at 12:05
@larsmans I updated the code with your suggestion, or at least my interpretation of it. — Ruairi O'Brien, Jan 23 '14 at 12:10
There's also no need to `sprintf` into a buffer, then copy that buffer to another buffer. — Fred Foo, Jan 23 '14 at 12:11
@UmNyobe Thanks for that suggestion. I am just unsure how I would use it. I will look in to it more. — Ruairi O'Brien, Jan 23 '14 at 12:12
Updated code again based on suggestions. Still needs a bit of work but getting there. I really appreciate all your comments but I realise now, due to the fact that this has all been through comments, that this question is not a good format for SO and will probably end up being closed. — Ruairi O'Brien, Jan 23 '14 at 12:36
@fotanus *NEVER* use the other 'n' functions!! `snprintf` is the only one that's actually safe. http://meyering.net/crusade-to-eliminate-strncpy/ — Roddy, Jan 23 '14 at 13:06

score 2 · Accepted Answer · answered Jan 23 '14 at 12:44

2

If you know that the result buffer will be bin enough (which you usually can ensure by allocating the maximum space necessary for any of the formats and validating your inputs before formatting them), you can do something like the following:

char* buffer = malloc(BIG_ENOUGH);
char* t = buffer;
t+=sprintf(t, "%d", sid->Revision);
t+=sprintf(t, "%d", sid->IdentifierAuthority.Value[5]);
for (i = 0; i < sid -> SubAuthorityCount; i++) {
    t += sprintf(t, "-%lu", sid -> SubAuthority[i]);
}
printf("Result: %s\n", buffer);

answered Jan 23 '14 at 12:44

Magnus Reftel

967
6
19

That's pretty neat, and works. One thing. To give back the result I used: sprintf(result, buffer); free(buffer); Is that correct or should I pass the buffer as the pointer to the result? – Ruairi O'Brien Jan 23 '14 at 13:03
1

Either let the caller pass the buffer and its size (and verify that the size is big enough before using it), or let the function return the buffer to the caller. Copying the contents of the buffer to some other buffer doesn't really solve anything - either the caller had to supply a big enough buffer (which has the same trade-offs as the first option), or the copy has to be returned to the caller (which has the same trade-offs as the second option). – Magnus Reftel Jan 23 '14 at 13:48

Roddy · Answer 2 · 2014-01-23T13:10:49.283

Looks all to complicated to me. I'd replace most of it with a single snprintf()

So this...

snprintf(revision, 2, "%d", sid -> Revision);
snprintf(identifierAuthority, 2, "%d", sid -> IdentifierAuthority.Value[5]);

/* Push prefix in to result buffer */
strcpy (result,sidStringPrefix);
/* Add revision so now should be S-{revision} */
strcat(result, revision);
/* Append another - symbol */
strcat(result, "-");
/* Add the identifier authority */
strcat(result, identifierAuthority);

...would become

snprintf("S-%d-%d", size, sid->Revision, sid->IdentifierAuthority.Value[5]);

Also, in your loop, you're not using size correctly.

      concatLength += snprintf(result + concatLength, size, "-%lu", sid -> SubAuthority[i]);

You should use size-concatLength so that the size reflects how much has already been written.

Oh, and this:-

     concatLength = snprintf(result, size, "%s-%lu", result, sid -> SubAuthority[i]);

.. is probably unsafe, as your destination is one of your source parameters. Generally, for the loop I;d use a solution like @MagnusReftel's.

Make that `snprintf`. **Never** use `sprintf`. OP did this right and you made it wrong in your answer. — R.. GitHub STOP HELPING ICE, Jan 23 '14 at 13:14
@R.. Already fixed. `snprintf` is the one safe `n` call. The others all stink. — Roddy, Jan 23 '14 at 13:17
I know I'm not supposed to leave these kind of comments but if I could have chosen 2 correct answers I would have. You showed me a lot. Thank you. — Ruairi O'Brien, Jan 23 '14 at 14:06

Building up a string in C. Am I doing it right?

2 Answers2