git_buf_vprintf fails when parsing repositoryformatversion = 0 on windows

[TNick]

Originally posted on stackoverflow

git describe returns v0.16.0-2393-gb72f5d4

On a Windows XP, 32-bit machine I've cloned the libgit2 repository using
git clone git://github.com/libgit2/libgit2.git trunk
Then I configured from trunk_build_debug for mingw using:
cmake ../trunk -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS=-g
and build-ed it with: make

With this newly build library I'm trying to create a new repository. So I'm using

    error_code = git_repository_init( &repo, path, 0 );
    if ( error_code != 0 )
    {
        const git_error * error = giterr_last();
       /* the text representation:
        ( error == NULL || error->message == NULL ? 
        "(no description)" : 
        error->message )
        */
    }

On first attempt this fails with the error message:
Git error -1: The global file '.config/git/config' doesn't exist: The system cannot find the file specified.

The .git directory is created and I can use git status to query it. Also, I have git installed in Program Files/git and it works just fine. The file that is mentioned does not exist in user directory, that is true. I'm not happy because I can't check the returned error code (the error may be "real").

So I've created an empty file in D:\Documents and Settings\User Name.config\git\config The .git directory is again created but I get the same -1 error code. This time giterr_last() returns NULL.

Debugging libgit2 I arrived at following failure:

#0  git_buf_vprintf (buf=0x23f3d8, format=0x6cb5226b "%s.%s", ap=0x23efc8 "À\217ß\025\210´ß\025") at $LIBGIT2\trunk\src\buffer.c:197
        args = 0x23efc8 "À\217ß\025\210´ß\025"
        len = -1
        expected_size = 10
#1  0x6cac6a35 in git_buf_printf (buf=0x23f3d8, format=0x6cb5226b "%s.%s") at $LIBGIT2\trunk\src\buffer.c:220
        r = 366985288
        ap = 0x23efc8 "À\217ß\025\210´ß\025"
#2  0x6cad0606 in config_parse (cfg_file=0x15dfcdf8, level=4) at $LIBGIT2\trunk\src\config_file.c:1024
        c = 114
        current_section = 0x15df8fc0 "core"
        var_name = 0x15dfb488 "repositoryformatversion"
        var_value = 0x15dfa510 "0"
        var = 0x15dfbf40
        existing = 0x1c2c42e
        buf = {ptr = 0x15dfc970 "core.repositoryf««««««««îþîþîþîþ", asize = 16, size = 0}
        result = 0
        pos = 0
        idx_i = 3
        cbuf = "[br", '\000' <repeats 43 times>, stripped
        p1 = 0x23efd4 "[br"
        p2 = 0x23efd7 ""
#3  0x6caceec8 in config_open (cfg=0x15dfcdf8, level=4) at $LIBGIT2\trunk\src\config_file.c:201
        res = 0
        b = 0x15dfcdf8
#4  0x6caccf80 in git_config_add_backend (cfg=0x15dfccc8, file=0x15dfcdf8, level=4, force=0) at $LIBGIT2\trunk\src\config.c:265
        internal = 0x513838a4
        result = 366988792
#5  0x6caccbe8 in git_config_add_file_ondisk (cfg=0x15dfccc8, path=0x15dfcda8 "$TEST_PATH/test_new_prj/.git/config", level=4, force=0) at $LIBGIT2\trunk\src\config.c:105
        file = 0x15dfcdf8
        res = 1823827983
#6  0x6cb04f85 in load_config (out=0x15dfaf8c, repo=0x15dfaf88, global_config_path=0x15dfa2a0 "$USER_PATH/.gitconfig", xdg_config_path=0x15dfb2c0 "$USER_PATH/.config/git/config", system_config_path=0x15dfcd30 "$PROGRAM_FILES/Git/etc/gitconfig") at $LIBGIT2\trunk\src\repository.c:467
        error = 0
        config_path = {ptr = 0x15dfcda8 "$TEST_PATH/test_new_prj/.git/config", asize = 56, size = 50}
        cfg = 0x15dfccc8
#7  0x6cb05162 in git_repository_config__weakptr (out=0x23f588, repo=0x15dfaf88) at $LIBGIT2\trunk\src\repository.c:523
        xdg_buf = {ptr = 0x15dfb2c0 "$USER_PATH/.config/git/config", asize = 56, size = 55}
        global_config_path = 0x15dfa2a0 "$USER_PATH/.gitconfig"
        system_config_path = 0x15dfcd30 "$PROGRAM_FILES/Git/etc/gitconfig"
        global_buf = {ptr = 0x15dfa2a0 "$USER_PATH/.gitconfig", asize = 48, size = 47}
        system_buf = {ptr = 0x15dfcd30 "$PROGRAM_FILES/Git/etc/gitconfig", asize = 40, size = 34}
        res = 366981144
        xdg_config_path = 0x15dfb2c0 "$USER_PATH/.config/git/config"
#8  0x6cb044d9 in load_config_data (repo=0x15dfaf88) at $LIBGIT2\trunk\src\repository.c:127
        is_bare = 366977464
        config = 0x3
#9  0x6cb04d1a in git_repository_open_ext (repo_ptr=0x23f714, start_path=0x15dfa240 "$TEST_PATH/test_new_prj/.git/", flags=1, ceiling_dirs=0x0) at $LIBGIT2\trunk\src\repository.c:384
        error = 0
        path = {ptr = 0x6cb60205 "", asize = 0, size = 0}
        parent = {ptr = 0x15dfa300 "$TEST_PATH/test_new_prj/", asize = 40, size = 39}
        repo = 0x15dfaf88
#10 0x6cb04d90 in git_repository_open (repo_out=0x23f714, path=0x15dfa240 "$TEST_PATH/test_new_prj/.git/") at $LIBGIT2\trunk\src\repository.c:398
No locals.
#11 0x6cb068ce in git_repository_init_ext (out=0x23f714, given_repo=0x15dfa178 "$TEST_PATH/test_new_prj", opts=0x23f660) at $LIBGIT2\trunk\src\repository.c:1256
        error = 0
        repo_path = {ptr = 0x15dfa240 "$TEST_PATH/test_new_prj/.git/", asize = 72, size = 44}
        wd_path = {ptr = 0x15dfa200 "$TEST_PATH/test_new_prj/", asize = 40, size = 39}
#12 0x6cb066e1 in git_repository_init (repo_out=0x23f714, path=0x15dfa178 "$TEST_PATH/test_new_prj", is_bare=0) at $LIBGIT2\trunk\src\repository.c:1210
        opts = {version = 1, flags = 196624, mode = 0, workdir_path = 0x0, description = 0x0, template_path = 0x0, initial_head = 0x0, origin_url = 0x0}

So git_buf_vprintf() computes an expected size based on the format (%s.%s)
const size_t expected_size = buf->size + (strlen(format) * 2);

repositoryformatversion = 0 is longer than this, so the call to p_vsnprintf() fails.
Changing the line to prevents the failure.
const size_t expected_size = buf->size + (strlen(format) * 16);
but I don't think is a proper solution (nor do I have other suggestion).

[carlosmn]

The first error string you can ignore. It is not the cause of the error, it's simply leaking; it seems we're not clearing the buffer correctly.

git_buf_vprintf only fails (returns -1) if the underlying system's vprintf tells it something went wrong. If there is not enough space vsnprintf tell us how long it should be, and we allocate that much space (rounded). Allocating more memory simply shouldn't have any effect on that. expected_size is simply a way to make most calls need a single memory allocation instead of having to try again.

Windows for some reason only ever returns -1, but in that case vscprintf is called, giving us the right return code again. Thus for Windows, we never return -1.

This could only happen if you're building inside MinGW (maybe cygwin?) and the library encounters an error, which the library wouldn't be able to handle in any case.

[TNick]

I am building inside MinGW, yes.

gcc.exe (Built by MinGW-builds project) 4.7.2
Copyright (C) 2012 Free Software Foundation, Inc.

So the problem is __mingw_vsnprintf?

[TNick]

How about:

int p_vsnprintf(char *buffer, size_t count, const char *format, va_list argptr)
{
#ifdef _MSC_VER
    int len;

    if (count == 0 ||
        (len = _vsnprintf_s(buffer, count, _TRUNCATE, format, argptr)) < 0)
            return _vscprintf(format, argptr);

    return len;
#else /* MinGW */
    int len = vsnprintf(buffer, count, format, argptr);
    if ( len < 0 )
        return count + count/2;
    else
        return len;
#endif
}

[carlosmn]

First of all, you need to start by telling us you're on MinGW, because that's not the same as Windows.

Hiding an error from the system and returning some random value isn't a viable solution. What it looks like is that MinGW is in fact using the vsnprintf from the MSCRT, so the define isn't needed at all and the function under MinGW should get the same broken status as on Windows. So try with simply the code inside the #ifdef before ignoring error codes from the OS.

[TNick]

First of all, you need to start by telling us you're on MinGW, because that's not the same as Windows.

My bad.

Hiding an error from the system and returning some random value isn't a viable solution.... So try with simply the code inside the #ifdef before ignoring error codes from the OS.

The error code means that the buffer is not large enough. Are there any other error cases?
The return code is not ignored, it is handled. The value is not random, is the size of the buffer plus half its size - a method to request a larger buffer.

git_buf_vprintf() would handle this well:

int git_buf_vprintf(git_buf *buf, const char *format, va_list ap)
{
    int len;
    const size_t expected_size = buf->size + (strlen(format) * 16);
    ENSURE_SIZE(buf, expected_size);
    while (1) {
        va_list args;
        va_copy(args, ap);
        len = p_vsnprintf(
            buf->ptr + buf->size,
            buf->asize - buf->size,
            format, args
        );
        if (len < 0) {
            git__free(buf->ptr);
            buf->ptr = git_buf__oom;
            return -1;
        }
        if ((size_t)len + 1 <= buf->asize - buf->size) {
            buf->size += len;
            break;
        }
        ENSURE_SIZE(buf, buf->size + len + 1);
    }
    return 0;
}

If I'm using the version for MSC an exception is raised: DLL entry point not found

[carlosmn]

-1 means format error and it's the only actual error you can get from it according to POSIX, which is what we're simulating with the p_ functions. If there is an actual error, your proposed solution would take us into an endless loop where we keep allocating more memory.

The issue is assuming vsnprintf behaves like POSIX instead of like MSCRT. Change that assumption instead of trying to assume what errors can happen.

[TNick]

link me, please, because the msdn page is telling us that:

Return Value

vsnprintf,_vsnprintf, and _vsnwprintf return the number of characters written if the number of characters to write is less than or equal to count; if the number of characters to write is greater than count, these functions return -1 indicating that output has been truncated. The return value does not include the terminating null, if one is written.

If buffer or format is NULL, or if count is less than or equal to zero, these functions invoke the invalid parameter handler, as described in Parameter Validation. If execution is allowed to continue, these functions return -1 and set errno to EINVAL.

The POSIX standard states that:

If an output error was encountered, these functions shall return a negative value. If the value of n is zero on a call to snprintf(), nothing shall be written, the number of bytes that would have been written had n been sufficiently large excluding the terminating null shall be returned, and s may be a null pointer.

note that vfprintf redirects to linked page.

I see no mention about -1 being a hint that a format error occurred. Is the linked page inaccurate?

So, maybe the issue is that I don't get what the purpose of p_vsnprintf() is. It is true that proposed solution may not return the size of the buffer that's needed the first time it is called, but p_vsnprintf() is called in a loop, anyway. The code that uses p_vsnprintf() does not set n to 0, so its not POSIX compliant anyway (again, according to the linked page).

[carlosmn]

Some places call it 'output error', others 'format error'. That is not the issue. p_ functions simulate what POSIX does so we can program the library with those assumptions.

Thus p_vsnprintf needs to return what vsnprintf would return on POSIX. The code in the _MSC_VER #ifdef does this. As it seems MinGW is using those functions underneath, the same functions (though apparently without the leading underscore, for whatever reason) need to be called so we get the result we're asking of it.

[carlosmn]

This compiles (and should work, but I have no MinGW environment handy), assuming you do have _vscprintf in the environment, and avoids assuming how the higher layers work by returning the length rather than some random number.

#ifdef _MSC_VER
int p_vsnprintf(char *buffer, size_t count, const char *format, va_list argptr)
{
    int len;

    if (count == 0 ||
        (len = _vsnprintf_s(buffer, count, _TRUNCATE, format, argptr)) < 0)
        return _vscprintf(format, argptr);

    return len;
}
#else /* MinGW */
#if HAVE__VSCPRINTF && defined(__MINGW32__) && !defined(__MINGW64_VERSION_MAJOR)
/* MinGW.org provided headers miss this prototype */
_CRTIMP int __cdecl __MINGW_NOTHROW _vscprintf(const char*, __VALIST);
#endif

int p_vsnprintf(char *buffer, size_t count, const char *format, va_list argptr)
{
    int len;

    if (count == 0 ||
        (len = vsnprintf(buffer, count, format, argptr)) < 0)
        return _vscprintf(format, argptr);

    return len;

}
#endif

[carlosmn]

Are you using an old version of MinGW? I can't reproduce their vsnprintf behaving like Windows. It seems from 3.15 onwards, they implement the C99 versions.

[carlosmn]

It's in fact mingw-runtime-3.14 when vsnprintf and friends started behaving like in C99. It was released in 2007 and it doesn't seem realistic to support anything older. If you do have a newer version of the runtime and it behaves like the MSCRT version, the bug would lie with them.

[TNick]

I am using the MinGW version that ships with Qt 5.0.1. info.txt file states:

!!!!: MinGW-builds-0.3.0
user: alexey
date: 01.17.2013-10:48:11

It seems that Qt 5 uses MinGW-builds.

[TNick]

I have tested provided patch and it works just fine.