Sprintf_s giving me weird boxes instead of strings

Question

#include <windows.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include "resource.h"


char* getString(HWND hwnd,int dlgItem) 
{
    int len = GetWindowTextLength(GetDlgItem(hwnd, dlgItem));
    if (len > 0)
    {
        TCHAR szBuffer[128] = { 0 };
        int s = GetDlgItemText(hwnd, dlgItem, szBuffer, len + 1);
        return szBuffer;
    }
    return "";
}

int getInt(HWND hwnd, int dlgItem)
{
    int len = GetWindowTextLength(GetDlgItem(hwnd, dlgItem));
    if (len > 0)
    {
        BOOL bsuccess;
        int Number = GetDlgItemInt(hwnd, dlgItem, &bsuccess, FALSE);
        return Number;
    }
    return 0;
}

char* _personRecord(HWND hwnd)
{
    int nameLength = GetWindowTextLength(GetDlgItem(hwnd, txtName)) + 1;
    int addressLength = GetWindowTextLength(GetDlgItem(hwnd, txtAddress)) + 1;
    int phoneLength = GetWindowTextLength(GetDlgItem(hwnd, txtPhone)) + 1;
    int emailLength = GetWindowTextLength(GetDlgItem(hwnd, txtEmail)) + 1;

    char* name = malloc(nameLength);
    name = getString(hwnd, txtName);
    int age = getInt(hwnd, txtAge);
    char* address = malloc(addressLength);
    address = getString(hwnd, txtAddress);
    int zip = getInt(hwnd, txtZip);
    char* phone= malloc(phoneLength);
    phone = getString(hwnd, txtPhone);
    char* email = malloc(emailLength);
    email = getString(hwnd, txtEmail);
    int length = nameLength + addressLength + phoneLength + emailLength + age + zip;
    char* personRecord = malloc(length);

    sprintf_s(personRecord , length, "Name: %s\nAge: %d\nAddress: %s\nZip: %d\nPhone: %s\nEmail: %s\n", name, age, address, zip, phone, email);

    OutputDebugString("\nStart*********\n");
    OutputDebugString(personRecord);
    OutputDebugString("\nEnd**********\n");
    return personRecord;

    free(name);
    free(address);
    free(phone);
    free(email);
    free(personRecord);
}

void saveFile(char* record)
{
    FILE* fp = NULL;
    if(fopen_s(&fp,"contacts.txt", "a") == 0)
    {
        fprintf(fp, "\n");
        fprintf(fp, record);
        fclose(fp);
    }
    else
    {
        OutputDebugString("Op failed");
    }
}

//Works 100 percent.
void clear(HWND hwnd)
{
    SetDlgItemText(hwnd, txtName, "");
    SetDlgItemText(hwnd, txtAge, "");
    SetDlgItemText(hwnd, txtAddress, "");
    SetDlgItemText(hwnd, txtZip, "");
    SetDlgItemText(hwnd, txtPhone, "");
    SetDlgItemText(hwnd, txtEmail, "");
}


BOOL CALLBACK EventHandler(HWND holdWindow, UINT Message, WPARAM wParam, LPARAM lParam)
{
    switch (Message)
    {
    case WM_INITDIALOG:
        break;
    case WM_COMMAND:
        switch (LOWORD(wParam))
        {
        case btnSave:
            saveFile(_personRecord(holdWindow));
            clear(holdWindow);
            break;
        case btnLoad:
            //print all records to console
            break;
        case btnClose:
            EndDialog(holdWindow, 0);
        }
        break;
    case WM_CLOSE:
        EndDialog(holdWindow, 0);
        break;
    default:
        return FALSE;
    }
    return TRUE;
}

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow)
{
    return DialogBox(hInstance, MAKEINTRESOURCE(IDD_DIALOG1), NULL, EventHandler);
}

First real crack at a C program that really does anything of note. I'm sure I'm doing a ton wrong with memory management but I'm a newbie so please go easy on me. As far as my problem goes, sprintf is turning those strings into boxes when I print them out and save them to the file. I thought it could be my getString methods that are causing this problem but if I print a variable out after it's been called it works fine in the debug. I've tried adjusting how I'm allocation personRecord several times including making it static and NULL. I've also tried printing them individually. I'm at a complete loss at this point.

Review [main difference between sprintf_s and sprintf is that sprintf_s takes a length parameter specifying the size of the output buffer in characters.](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/sprintf-s-sprintf-s-l-swprintf-s-swprintf-s-l?view=vs-2019) and `sprintf_s(personRecord , length, "Name: %s\n ...`. see if that helps. — chux - Reinstate Monica, Nov 26 '19 at 03:37
The getString function returns szBuffer which is local to this function. You can not return pointers to local variables. See https://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope — Nitesh, Nov 26 '19 at 03:37
So I need to make some global variables and set them manually instead of creating the get string function? How come I can debug output the strings before they hit sprint? Is it because of what that analogy said? — Markusreynolds1989, Nov 26 '19 at 12:52
`sprintf_s()`? If you're being directed to use such "safer" functions, you should be aware that the functions in Annex K of the C standard are optional, and effectively only Microsoft has implemented them - and Microsoft's implementations are not in compliance with the C standard anyway. Effectively, you are writing non-portable code. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1967.htm — Andrew Henle, Nov 26 '19 at 14:25
Does this answer your question? [Can a local variable's memory be accessed outside its scope?](https://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope) — Raymond Chen, Nov 26 '19 at 14:44
I really appreciate everyone's answers, from what I could tell is that my implementation was okay, the problem was that I wasn't allocating enough memory in the getString function which was causing the issue. When I dynamically added the memory using malloc it worked correctly. I also appreciate the heads up from above about microsofts non-portable functions. Would this apply in this situation as I am writing using the Win32 API? Wouldn't that in of itself be considered non-portable? — Markusreynolds1989, Nov 26 '19 at 21:50

Markusreynolds1989 · Accepted Answer · 2019-11-26T21:52:02.613

0

char* szBuffer[1024] = { 0 };

TCHAR was the wrong type and 128 was not enough memory to save the string properly. I adjusted how much memory could be used and it fixed the issue. I think the correct answer would be to malloc the data to the length of the string I took from the window but I'm not sure entirely. Thanks for everyone who posted.

char* getString(HWND hwnd,int dlgItem) 
{
    int len = GetWindowTextLength(GetDlgItem(hwnd, dlgItem));
    if (len > 0)
    {
        char* record = malloc(len * sizeof *record + 1);
        int s = GetDlgItemText(hwnd, dlgItem, record, len + 1);
        return record;
        free(record);
    }
    return "";
}

edited Nov 26 '19 at 21:52

answered Nov 26 '19 at 14:03

Markusreynolds1989

11
3

Note that `free(record)` won't do anything, because it comes directly after a `return`; it's dead code. You wouldn't want to do that anyway because it would invalidate the pointer you're returning. – Mark Ransom Nov 26 '19 at 22:48
Yeah I thought as much, i appreciate the feedback. I'm in the process of cleaning it up with better typing and correct memory allocation. – Markusreynolds1989 Nov 27 '19 at 01:23

Sprintf_s giving me weird boxes instead of strings

1 Answers1