threads / patch / 25124

patch, 2 partsFix uninitialized memory read and comment typo

Subject: [PATCH 0/2] Fix uninitialized memory read and comment typo

## tl;dr

9 messages between Sep 16, 2010 and Sep 17, 2010. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Pat Notz· Sep 16, 2010, 20:53 UTC · lore
One-liner corrections.
Pat Notz (2):
  dir.c: fix uninitialized memory warning
  strbuf.h: fix comment typo
 dir.c    |    2 +-
 strbuf.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
-- 
1.7.2.3
Pat Notz· Sep 16, 2010, 20:53 UTC · re: Pat Notz · lore

[PATCH 1/2] dir.c: fix uninitialized memory warning

GCC 4.4.4 on MacOS warns about potential use of uninitialized memory.
Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to dir.c +1 −1
diff --git a/dir.c b/dir.c
index 133f472..d1e5e5e 100644
--- a/dir.c
+++ b/dir.c
@@ -232,7 +232,7 @@ int add_excludes_from_file_to_list(const char *fname,
 {
 	struct stat st;
 	int fd, i;
-	size_t size;
+	size_t size = 0;
 	char *buf, *entry;
 
 	fd = open(fname, O_RDONLY);

-- 
1.7.2.3
Ævar Arnfjörð Bjarmason· Sep 16, 2010, 23:13 UTC · re: Pat Notz · lore

Re: [PATCH 1/2] dir.c: fix uninitialized memory warning

On Thu, Sep 16, 2010 at 20:53, Pat Notz <patnotz@gmail.com> wrote:
Show 18 quoted lines
> GCC 4.4.4 on MacOS warns about potential use of uninitialized memory.
>
> Signed-off-by: Pat Notz <patnotz@gmail.com>
> ---
>  dir.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 133f472..d1e5e5e 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -232,7 +232,7 @@ int add_excludes_from_file_to_list(const char *fname,
>  {
>        struct stat st;
>        int fd, i;
> -       size_t size;
> +       size_t size = 0;
>        char *buf, *entry;

What does the GCC warning say exactl? I.e. what line does it complain about?

Maybe this is a logic error introduced in v1.7.0-rc0~25^2? I haven't checked.

Nguyen Thai Ngoc Duy· Sep 16, 2010, 23:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/2] dir.c: fix uninitialized memory warning

2010/9/17 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
Show 25 quoted lines
> On Thu, Sep 16, 2010 at 20:53, Pat Notz <patnotz@gmail.com> wrote:
>> GCC 4.4.4 on MacOS warns about potential use of uninitialized memory.
>>
>> Signed-off-by: Pat Notz <patnotz@gmail.com>
>> ---
>>  dir.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 133f472..d1e5e5e 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -232,7 +232,7 @@ int add_excludes_from_file_to_list(const char *fname,
>>  {
>>        struct stat st;
>>        int fd, i;
>> -       size_t size;
>> +       size_t size = 0;
>>        char *buf, *entry;
>
> What does the GCC warning say exactl? I.e. what line does it complain
> about?
>
> Maybe this is a logic error introduced in v1.7.0-rc0~25^2? I haven't
> checked.

I don't see any case that "size" can be used uninitialized. Maybe the compiler was confused by

if (!check_index ||
    (buf = read_skip_worktree_file_from_index(fname, &size)) == NULL)
        return -1;

I wouldn't hurt though to initialize it early, even just to stop the compiler from complaining.

-- 
Duy
Pat Notz· Sep 17, 2010, 00:32 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 1/2] dir.c: fix uninitialized memory warning

On Thu, Sep 16, 2010 at 5:26 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
Show 23 quoted lines
> 2010/9/17 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>> On Thu, Sep 16, 2010 at 20:53, Pat Notz <patnotz@gmail.com> wrote:
>>> GCC 4.4.4 on MacOS warns about potential use of uninitialized memory.
>>>
>>> Signed-off-by: Pat Notz <patnotz@gmail.com>
>>> ---
>>>  dir.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/dir.c b/dir.c
>>> index 133f472..d1e5e5e 100644
>>> --- a/dir.c
>>> +++ b/dir.c
>>> @@ -232,7 +232,7 @@ int add_excludes_from_file_to_list(const char *fname,
>>>  {
>>>        struct stat st;
>>>        int fd, i;
>>> -       size_t size;
>>> +       size_t size = 0;
>>>        char *buf, *entry;
>>
>> What does the GCC warning say exactl? I.e. what line does it complain
>> about?
Here's the output:

make V=1 -j2 all gcc -o dir.o -c -g -O2 -Wall -I. -I/opt/local/include -DUSE_ST_TIMESPEC -DSHA1_HEADER='<openssl/sha.h>' -DNO_MEMMEM dir.c dir.c: In function 'add_excludes_from_file_to_list': dir.c:235: warning: 'size' may be used uninitialized in this function

Show 11 quoted lines
>>
>> Maybe this is a logic error introduced in v1.7.0-rc0~25^2? I haven't
>> checked.
>
> I don't see any case that "size" can be used uninitialized. Maybe the
> compiler was confused by
>
> if (!check_index ||
>    (buf = read_skip_worktree_file_from_index(fname, &size)) == NULL)
>        return -1;
>
No, line 245: if(size==0)
Show 5 quoted lines
> I wouldn't hurt though to initialize it early, even just to stop the
> compiler from complaining.
> --
> Duy
>
Nguyen Thai Ngoc Duy· Sep 17, 2010, 01:04 UTC · re: Pat Notz · lore

Re: [PATCH 1/2] dir.c: fix uninitialized memory warning

On Fri, Sep 17, 2010 at 10:32 AM, Pat Notz <patnotz@gmail.com> wrote:
Show 9 quoted lines
>> I don't see any case that "size" can be used uninitialized. Maybe the
>> compiler was confused by
>>
>> if (!check_index ||
>>    (buf = read_skip_worktree_file_from_index(fname, &size)) == NULL)
>>        return -1;
>>
>
> No, line 245: if(size==0)

The only chance for that line to be executed is read_skip_*() is executed and returns non-NULL buf. read_skip*() returns a non-NULL buffer at the end of function and does set size right before returning.

To me it looks like a false alarm. But again, no objection to the patch.
-- 
Duy
Pat Notz· Sep 17, 2010, 01:13 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 1/2] dir.c: fix uninitialized memory warning

On Thu, Sep 16, 2010 at 7:04 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
Show 17 quoted lines
> On Fri, Sep 17, 2010 at 10:32 AM, Pat Notz <patnotz@gmail.com> wrote:
>>> I don't see any case that "size" can be used uninitialized. Maybe the
>>> compiler was confused by
>>>
>>> if (!check_index ||
>>>    (buf = read_skip_worktree_file_from_index(fname, &size)) == NULL)
>>>        return -1;
>>>
>>
>> No, line 245: if(size==0)
>
> The only chance for that line to be executed is read_skip_*() is
> executed and returns non-NULL buf. read_skip*() returns a non-NULL
> buffer at the end of function and does set size right before
> returning.
>
> To me it looks like a false alarm. But again, no objection to the patch.

I agree that it's a false alarm which is why I wasn't too interested in looking into it very deeply. Just looking to keep the code warning free is all.

> --
> Duy
>
Pat Notz· Sep 17, 2010, 17:23 UTC · re: Pat Notz · lore

Re: [PATCH 1/2] dir.c: fix uninitialized memory warning

For anyone who care, this warning was actually emitted by the version of GCC that ships with MacOS 10.5: i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5493).

GCC 4.4.4 does *not* git this warning.
Sorry for the confusion, my IDE was using a different $PATH than my shell.
On Thu, Sep 16, 2010 at 7:13 PM, Pat Notz <patnotz@gmail.com> wrote:
Show 27 quoted lines
> On Thu, Sep 16, 2010 at 7:04 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> On Fri, Sep 17, 2010 at 10:32 AM, Pat Notz <patnotz@gmail.com> wrote:
>>>> I don't see any case that "size" can be used uninitialized. Maybe the
>>>> compiler was confused by
>>>>
>>>> if (!check_index ||
>>>>    (buf = read_skip_worktree_file_from_index(fname, &size)) == NULL)
>>>>        return -1;
>>>>
>>>
>>> No, line 245: if(size==0)
>>
>> The only chance for that line to be executed is read_skip_*() is
>> executed and returns non-NULL buf. read_skip*() returns a non-NULL
>> buffer at the end of function and does set size right before
>> returning.
>>
>> To me it looks like a false alarm. But again, no objection to the patch.
>
> I agree that it's a false alarm which is why I wasn't too interested
> in looking into it very deeply.  Just looking to keep the code warning
> free is all.
>
>> --
>> Duy
>>
>
Pat Notz· Sep 16, 2010, 20:53 UTC · re: Pat Notz · lore

[PATCH 2/2] strbuf.h: fix comment typo

Signed-off-by: Pat Notz <patnotz@gmail.com>
---
 strbuf.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to strbuf.h +1 −1
diff --git a/strbuf.h b/strbuf.h
index fac2dbc..675a91f 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -16,7 +16,7 @@
  *
  * 2. the ->buf member is a byte array that has at least ->len + 1 bytes
  *    allocated. The extra byte is used to store a '\0', allowing the ->buf
- *    member to be a valid C-string. Every strbuf function ensure this
+ *    member to be a valid C-string. Every strbuf function ensures this
  *    invariant is preserved.
  *
  *    Note that it is OK to "play" with the buffer directly if you work it

-- 
1.7.2.3

← back to recent threads