rafaelsteil / libcgi Goto Github PK

It's typical (if not only) use case:

while ((value = cgi_param_multiple(name)) != NULL) {
// processing here...
}

will cause endless loop when name is found at the end of the CGI parameter list.

In cgi_process_form() the memory allocated for HTTP POST data input is not freed. See:

        char *post_data;
        char *tmp_data;
        int content_length;

        tmp_data = getenv("CONTENT_LENGTH");
        if (tmp_data == NULL)
            return NULL;

        content_length = atoi(tmp_data);

        post_data = (char *)malloc(content_length + 1);
        if (post_data == NULL)
            libcgi_error(E_MEMORY, "%s, line %s", __FILE__, __LINE__);

        fread(post_data, content_length, 1, stdin);
        post_data[content_length] = '\0';

        return process_data(post_data, &formvars_start, &formvars_last, '=', '&');

Adding an additional pointer for return and freeing memory before return could solve this:

        formvars *ret;

        /* ... */

        ret = process_data(post_data, &formvars_start, &formvars_last, '=', '&');
        free( post_data );
        return ret;

Would improve handling large forms to be processed in small memory environments.

When running clang-tidy on the code, warnings are thrown for the file base64.c, including null pointer dereference and undefined behavior. This should probably be investigated?

libcgi contains an implementation of the md5 hash function, however its not used inside the library itself and the status regarding license or potential security issues (despite the weekness of md5 itself) is not clear. If a user needs md5 he or she could get it from a specialized library.

(Comments welcome.)

We come across some memory leak issues in your library, and have it fixed.

As the library is found home in github (recently), I would like to drop a note here:

1. cgi.c, cgi_process_form()
   post_data is malloc() within POST request handler, so it should be free after process_data().
   We come across cases with huge CGI data, which leave a bad memory usage foot print right there.

2. cgi.c process_data()
   data->value shouldn't be malloc(), since cgi_unescape_special_chars() will malloc() by itself and the memory is returned. It should simply be:

data->value = cgi_unescape_special_chars(query);

Currently there's libcgi_error() which is used in numerous places around the whole library. If the global variable cgi_display_errors contains true it prints HTML markup to stdout containing the error message. If the passed error code is E_FATAL or E_MEMORY (defined in src/error.h) it even calls cgi_end() and exit().

Besides the obvious sudden exit which may be bad (do we need this?) I consider it bad to print anything to stdout where my content should go to. The used HTML tags could be invalid if not printed at the right time or even break correct markup. (They do e.g. when using sessions if there's a session cookie but no session file.)

A flexible solution could work like in libabc or we could just print to stderr, which some webservers can append to their logfiles (others return HTTP status 500 or mix stdout/stderr and return it to the browser).

The function cgi_unescape_special_chars() allocates some memory based on the length of the input string and returns the pointer to this new allocated memory as output. The function is used twice in LibCGI and in neither case the memory is freed again although not used further.

First is in cgi_get_cookies() where the pointer taking the return argument is overwritten multiple times before the memory can be freed.

Second is in process_data() where the result is directly fed in another function giving you no chance to free it again.

In environments with small memory this leads to problems processing large form data with lots of key/value pairs.

In "list.c", slist_add() could be greatly simplified if we ignore the order - appending. This is how the code could look if we prepend to the list instead:

    void slist_add(formvars *item, formvars **list)
    {
        item->next = *list;
        *list = item;
    }

The fix for #20 in a4f2e93 introduced a regression. The list pointer (**start) is not updated correctly anymore.

While working with sessions and using cgi_session_alter_var() before cgi_redirect() I noticed the former calling sess_file_rewrite() which seems plausible. However sess_file_rewrite() calls cgi_init_headers() for no obvious reason leading to cgi_redirect() complaining about already initialized headers.

I suggest removing the cgit_init_headers() call, but I'm not entirely sure if this would have some side effects I could not see?!

I propose switching the build system from Autotools to CMake. Here is why:

• I'm using CMake for years now and I'm familiar with the details how to use it
• I would have to learn Autotools from scratch to fix #5 or to make a new release tarball
• CMake supports cross compiling and is a modern build system supported by embedded build projects like ptxdist or buildroot
• I have already written basic CMake support in my personal fork: https://github.com/LeSpocky/libcgi/tree/cmake
• I have already written unit tests based on CTest which could be easily integrated then: https://github.com/LeSpocky/libcgi/tree/tests

Some things would still have to be done:

• support for pkg-config to make it easier for other projects to use libcgi
• use CPack to create a release tarball for milestone v1.1
• remove old Autotools files

A typical consumer of the library takes the header file(s) and some binaries, and can develop her application on top of that. If the API documentation would be included in the header as well, that would be sufficient, no need to look into things created by Doxygen or anything else. However Doxygen documentation can also be produced from the header instead of the source, it just does not matter. So be nice and move all the API documentation to the header file(s).

While testing a fix for #25 I saw it does not solve the problem I actually try to solve. 😥

In a classic CGI the process is started, some things are done, the process ends, everything not released by yourself is cleaned up by the operating system. Now if you want to use libcgi together with FastCGI (this requires patches not in mainline yet) the process never ends, but the whole thing is enclosed by an endless loop. In this case the library MUST do proper bookkeeping and cleanup.

So when using libcgi in such a loop the first call to cgi_session_start() sets sess_initialized to true, every following call fails, because sess_initialized is never reset to false, which makes the whole session thing unusable.

There is a second variable sess_finitialized which is never tested and only set to false in cgi_session_destroy(). So even if this is meant to be the same as sess_initialized it would not work, because cgi_session_destroy() is not called every time. Actually most of the time you don't want to destroy your session, but keep it.

So there needs to be a way for resetting those flags and find a proper name for this function.

If the list contains only one element the following line segfaults:

if (!strcasecmp((*start)->next->name, name)) {

With only one element (*start)->next is NULL and there's no check for that → segfault.

In slist_add() there's no check on the contents of item->name or item->value so items with name or value set to NULL can be added to the list.

The functions slist_delete() and slist_item() access item->name which would cause a segfault if name is set to NULL.

Additionally in slist_item() also item->value is accessed and can be NULL, here:

return (!begin->value[0] ? NULL : begin->value);

While libcgi does not add items with empty names by itself, it may add items with empty values. For example on POST requests with empty text boxes a QUERY_STRING can be like this:

foo=&bar=baz

The segfault happens in the above mentioned line in slist_item(), if the list is then accessed via cgi_param( "foo" ) 💥

I suggest we allow the value to be set to NULL and return NULL in slist_item() in this case.

The question remains if we should also allow adding items with empty name in slist_add(), but that's subject of another topic. 😉

On an empty list libcgi segfaults in slist_delete() here:

if (!strcasecmp((*start)->name, name)) {

(*start) is NULL here. This can be easily checked and avoided!

The global variable sess_fname is a pointer to dynamically allocated memory which happens either in sess_generate_id() or in cgi_session_start(). However this memory is never freed.

That API function is problematic in multiple ways:

• exit terminates the whole process, from a library, … well you don't do that, pass your error codes and let the application decide!
• cgi_init_headers() is called, if you want or not
• no actual error handling, which could lead to all kinds of null pointer dereferences, use after free, and so on :-/
• all depending on global variable cgi_display_errors (initialized with 1 aka true)
  • if cgi_display_errors is set, the process will be terminated (see above)
  • if cgi_display_errors is not set, libcgi_error() returns short and there's actually no error handling at all (see above)
• the function is exposed to the API, so a consumer could call it: WHY? o.O

NOTE: just avoiding libcgi_error() does not solve that missing error handling!

Originally created at http://sourceforge.net/p/libcgi/bugs/7/

when do cross compile:
./configure --prefix=$HOME/develop/crosstool-ng/x-tools/libcgi --build=i686-pc-cygwin --target=arm-xscale-linux-gnueabi --host=arm-xscale-linux-gnueabi
make CC=arm-xscale-linux-gnueabi-gcc
then do:
make install
but when, my here, not create the lib and include folder under prefix folder:
$HOME/develop/crosstool-ng/x-tools/libcgi
then:
make install will just copy files into file (lib and include), not those folder:
CLi@PC-CLI-1 ~/develop/libcgi/libcgi-1.0
$ make install
cp src/libcgi.a /home/CLi/develop/crosstool-ng/x-tools/libcgi/lib
cp src/libcgi.so /home/CLi/develop/crosstool-ng/x-tools/libcgi/lib
cp src/cgi.h /home/CLi/develop/crosstool-ng/x-tools/libcgi/include
cp src/session.h /home/CLi/develop/crosstool-ng/x-tools/libcgi/include
CLi@PC-CLI-1 ~/develop/libcgi/libcgi-1.0
$ ll /home/CLi/develop/crosstool-ng/x-tools/libcgi
total 48K
-rwxr-xr-x 1 CLi Domänen-Benutzer 1.1K Aug 6 12:59 include*
-rw-r--r-- 1 CLi Domänen-Benutzer 44K Aug 6 12:59 lib
so, solution:
change Makefile, from:
install:
cp src/libcgi.a $(LIBDIR)
cp src/libcgi.so $(LIBDIR)
cp src/cgi.h $(INCDIR)
cp src/session.h $(INCDIR)
to:
install:
mkdir -p $(LIBDIR)
cp src/libcgi.a $(LIBDIR)
cp src/libcgi.so $(LIBDIR)
mkdir -p $(INCDIR)
cp src/cgi.h $(INCDIR)
cp src/session.h $(INCDIR)
can fix this bug.
but I do not know how to edit the configue to finally fix this bug.
so need author to do something to fix this bug.