Multi-threaded mayhem

[IceDane]

I'm programming a multi-threaded, simple port scanner to try and familiarize myself with multi-threading, and I've run into a problem. I'm passing a pointer to a struct(Allocated on the heap) as the argument, then dereferencing the pointer in the thread callback and trying to use it to pass info to the thread, such as IP, ports and so on. However, I *think* the threads are segfaulting when it tries to dereference the pointer, but I can't really debug it, as I've never debugged threads and even when I tried doing so with gdb in codeblocks, it seemed to return fine.

The way it 'doesn't work' is basically by not even printing the sPort and ePort variables, or anything else after that, as if it's shut down.

Here's the code:

#include <stdio.h>
#include <stdlib.h>
#include <windows.h>
#include <process.h>

#define ecs() EnterCriticalSection(&cSection);
#define lcs() LeaveCriticalSection(&cSection);

struct sInfo
{
    unsigned int   startPort;
    unsigned int   noPorts;
    char           *host;
};

CRITICAL_SECTION cSection;

__stdcall void scanThread(void * Param)
{
    struct sockaddr_in remoteHost;

    struct sInfo sinfo  = *((struct sInfo*)Param); // Dereferencing the pointer. Wrong ? Cast to struct pointer, dereference pointer?


    int sPort = sinfo.startPort,
                ePort = sPort + sinfo.noPorts,
                theSocket;

    WSADATA wsaData;

    WSAStartup(MAKEWORD(2,0), &wsaData);


    ecs();

    printf("Starting port: &#37;d\n", sPort);

    lcs();



    remoteHost.sin_family = AF_INET;
    if ((remoteHost.sin_addr.s_addr = inet_addr(sinfo.host)) == -1)
    {
        ecs();

        printf("Could not resolve host.\n");

        lcs();

        _endthreadex(0);
    }

    memset(remoteHost.sin_zero, 0, 8);

    theSocket = socket(AF_INET, SOCK_STREAM, 0);

    if (theSocket == INVALID_SOCKET)
    {
        ecs();

        printf("Could not create socket: %d\n", WSAGetLastError());

        lcs();

        _endthreadex(0);
    }




    for (; sPort < ePort; sPort++)
    {
        remoteHost.sin_port = htons(sPort);


        if (connect(theSocket, (struct sockaddr*)&remoteHost, sizeof(struct sockaddr)))
        {
            ecs();

            printf("Port %d: CLOSED\n", sPort);

            lcs();
        }
        else
        {
            printf("Port %d: OPEN\n", sPort);

        }
    }


    closesocket(theSocket);
    WSACleanup();

    _endthreadex(0);
}


int main(int argc, char  **argv)
{
    if (argc < 4)
    {
        printf("Usage:\n"\
               "\tportscanner IP STARTPORT ENDPORT [NO.THREADS]\n\n");

        return 0;
    }

    int startPort = atoi(argv[2]), endPort = atoi(argv[3]),
                    portsPerThread, noThreads = 5, i = 0, x = 0;



    HANDLE *threadHandles;


    if (startPort > endPort)
    {
        printf("The start port must be smaller than the end port.\n");

        return 0;
    }

    if (argc >= 5) noThreads = atoi(argv[4]);

    threadHandles = malloc(noThreads * sizeof(HANDLE));


    portsPerThread = ((endPort - startPort)+1) / noThreads;




    printf("Number of threads: %d\n"
           "Ports per thread:  %d\n", noThreads, portsPerThread);


    // Array of pointers to struct sInfo.
    struct sInfo *scanInfo[noThreads];





    InitializeCriticalSection(&cSection);
    for (i = 0; x < noThreads; i += portsPerThread )
    {
        // Allocate memory at each pointer.
        scanInfo[x] = malloc(sizeof(struct sInfo));
        scanInfo[x]->noPorts = portsPerThread;
        scanInfo[x]->host    = malloc(sizeof(char) * 100);

        strcpy(scanInfo[x]->host, argv[1]);

        scanInfo[x]->startPort = i;


        threadHandles[x++] = (HANDLE)_beginthreadex(NULL,
                             0,
                             scanThread,
                             (void*)scanInfo[x], // Pass pointer to thread, here.                             0,
                             NULL);
    }


    WaitForMultipleObjects(noThreads,
                           threadHandles,
                           TRUE,
                           INFINITE);


    free(threadHandles);

    for(i = 0; i < noThreads; i++)
    {
        free(scanInfo[x]->host);
        free(scanInfo[x]);
    }

    //free(scanInfo);
    DeleteCriticalSection(&cSection);


    return 1;

}

[Dino]

Won't this:

  for (i = 0; noThreads--; i += portsPerThread )

cause this

WaitForMultipleObjects(noThreads,
                           threadHandles,
                           TRUE,
                           INFINITE);

to work differently from what you want?

Todd

[IceDane]

Won't this:

  for (i = 0; noThreads--; i += portsPerThread )

cause this

WaitForMultipleObjects(noThreads,
                           threadHandles,
                           TRUE,
                           INFINITE);

to work differently from what you want?

Todd

Hell yeah it will. That seems to have fixed it, thanks a bunch. Can't believe I missed that.

[IceDane]

I've run into a new problem, and I've replaced the updated code with the old code in the original post so I don't make this thread a thousand pages.

Anyway, I figured out that the threads had issues with synchronization, that is, using the same starting port as they were getting it from global memory, which made a lot of threads use same starting ports and thus ruining the point of the program.

So I figured I'd create an array of pointers, which I have, and then pass the pointers one by one to the threads, so I'd definitely be making them access separate memory. However, when trying to dereference the pointer in the thread proc, it seems to segfault.

I'm not too good with pointer arithmetic, when it goes that deep, to be honest, and I'd love it if someone could explain why this isn't working. Also, if you have any links on some more 'advanced' pointer arithmetic, which has detailed instructions on dereferencing n-duple pointers, and some thumb rules, I'd be eternally grateful. I've been googling for some info on it to fix my problem, but none of it was advanced enough to help me out.

[iMalc]

Do WSAStartup & WSACleanup need to be called for each thread? I think it is only once for the entire app.

You're not cosing the handles to the threads after waiting on them. Since you're freeing them, you may as well close them first.

Using C99 huh. Interesting choice that you've chosen to use malloc in one case, but a variable sized array in the other place.

Were you planning on using that critical section anywhere? It doesn't appear as if you need it, so you may as well get rid of it.

This is a no-no:

        threadHandles[x++] = (HANDLE)_beginthreadex(NULL,
                             0,
                             scanThread,
                             (void*)scanInfo[x], // Pass pointer to thread, here.
                             NULL);

Look how you're using x twice on that line. Just do the increment in the for-loop instead like this:

    for (i = 0; x < noThreads; i += portsPerThread, ++x )

Note that you had already set i to zero before the for-loop so you're doing it twice which is unnecessary, and inconsistent with what you're doing with x.

[Codeplug]

>> __stdcall void scanThread(void * Param)
should be
unsigned __stdcall scanThread(void * Param)

>> ...so you may as well get rid of it.
It's being used to mutex access to stdout. Nothing wrong with that.

gg

[IceDane]

Do WSAStartup & WSACleanup need to be called for each thread? I think it is only once for the entire app.

You're not cosing the handles to the threads after waiting on them. Since you're freeing them, you may as well close them first.

Using C99 huh. Interesting choice that you've chosen to use malloc in one case, but a variable sized array in the other place.

Were you planning on using that critical section anywhere? It doesn't appear as if you need it, so you may as well get rid of it.

This is a no-no:

        threadHandles[x++] = (HANDLE)_beginthreadex(NULL,
                             0,
                             scanThread,
                             (void*)scanInfo[x], // Pass pointer to thread, here.
                             NULL);

Look how you're using x twice on that line. Just do the increment in the for-loop instead like this:

    for (i = 0; x < noThreads; i += portsPerThread, ++x )

Note that you had already set i to zero before the for-loop so you're doing it twice which is unnecessary, and inconsistent with what you're doing with x.

Like Codeplug pointed out, I'm using them to synchronize access to stdout so I don't get garbage when I print debug statements. I've macroed EnterCriticalSection(&cSection) to ecs();, and LeaveCriticalSection(&cSection); to lcs(); because it's incredibly annoying to type the whole thing out every time I need to print something.

Anyway - thanks for the pointer about using x - I was wondering why it said using x in this context was undefined.

However, have you any idea why it is segfaulting?

[Codeplug]

        threadHandles[x++] = (HANDLE)_beginthreadex(NULL,
                             0,
                             scanThread,
                             (void*)scanInfo[x], // Pass pointer to thread, here.
                             NULL);

I believe iMalc already pointed it out. Every thread is getting an invalid pointer. Which you then dereference with
>> struct sInfo sinfo = *((struct sInfo*)Param);

gg

[IceDane]

        threadHandles[x++] = (HANDLE)_beginthreadex(NULL,
                             0,
                             scanThread,
                             (void*)scanInfo[x], // Pass pointer to thread, here.
                             NULL);

I believe iMalc already pointed it out. Every thread is getting an invalid pointer. Which you then dereference with
>> struct sInfo sinfo = *((struct sInfo*)Param);

gg

Hmm, that seems to be it. I compiled it before and it didn't work. Maybe I didn't compile.

Thanks for the help guys.