[Libusbx-devel] 3 warnings in git master from Xcode

[Sean McBride]

Hi all,

With 1.0.18 coming, I though I'd try building git master with the latest Xcode. I see 3 warnings:

(1) core.c:1910:27: Missing field 'tv_usec' initializer

can we just change:
struct timeval tv = { 0, };
to:
struct timeval tv = { 0, 0 };

is there any platform where this struct is not 2 elements?


(2) os/darwin_usb.c:531:10: Implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int'

A cast would silence this, but actually I worry the last line of the function is returning entirely the wrong thing. Shouldn't it return 'ret' not 'len'? The docs for 'get_config_descriptor' say 'Return 0 on success or a LIBUSB_ERROR code on failure.'


(3) core.c:1163:7: Use of memory after it is freed

This could be a false positive as it's from the static analyzer. It's code path dependant, and really you need the Xcode GUI to follow the flow. Nathan, could you look?

Cheers,

[Tim Roberts]

Post by Sean McBride
(1) core.c:1910:27: Missing field 'tv_usec' initializer
struct timeval tv = { 0, };
struct timeval tv = { 0, 0 };
is there any platform where this struct is not 2 elements?

Even if there isn't, the standard guarantees that those two constructs
are identical. Any elements that don't have initializer values get zeroed.

In other places (see line 2156), the code assumes that the structure
contains tv_sec and tv_usec, so there must be at least two elements.
Thus, your change is safe, although superfluous.

[Nathan Hjelm]

Post by Tim Roberts

Post by Sean McBride
(1) core.c:1910:27: Missing field 'tv_usec' initializer
struct timeval tv = { 0, };
struct timeval tv = { 0, 0 };
is there any platform where this struct is not 2 elements?

Even if there isn't, the standard guarantees that those two constructs
are identical. Any elements that don't have initializer values get zeroed.

Not quite. This is true for global/static variables. Anything allocated on the stack that is not explicitly initialized gets whatever was on already the stack.

Post by Tim Roberts
In other places (see line 2156), the code assumes that the structure
contains tv_sec and tv_usec, so there must be at least two elements.
Thus, your change is safe, although superfluous.
--
Providenza & Boekelheide, Inc.
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
libusbx-devel mailing list
https://lists.sourceforge.net/lists/listinfo/libusbx-devel

[Tim Roberts]

Post by Nathan Hjelm

Post by Tim Roberts
Even if there isn't, the standard guarantees that those two constructs
are identical. Any elements that don't have initializer values get zeroed.

Not quite. This is true for global/static variables. Anything allocated on the stack that is not explicitly initialized gets whatever was on already the stack.

Sorry, but this is incorrect. What you say is true if I omit the
initializer list completely. But if I provide a PARTIAL initializer
list, then the rest of the structure is zeroed, regardless of the
storage class.

*C99 Standard 6.7.8.21*

If there are fewer initializers in a brace-enclosed list than there
are elements or members of an aggregate, or fewer characters in a
string literal used to initialize an array of known size than there
are elements in the array, the remainder of the aggregate shall be
initialized implicitly the same as objects that have static storage
duration.


C++ words it a little differently, but the concept is the same. You can
always use this to zero-initialize a structure or array:

some_struct s = {0};

[Nathan Hjelm]

Post by Sean McBride
Hi all,
(1) core.c:1910:27: Missing field 'tv_usec' initializer
struct timeval tv = { 0, };
struct timeval tv = { 0, 0 };
is there any platform where this struct is not 2 elements?

Nope. struct timeval is POSIX (http://pubs.opengroup.org/onlinepubs/7999959899/basedefs/sys/time.h.html) and always has exactly 2 members.

Post by Sean McBride
(2) os/darwin_usb.c:531:10: Implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int'
A cast would silence this, but actually I worry the last line of the function is returning entirely the wrong thing. Shouldn't it return 'ret' not 'len'? The docs for 'get_config_descriptor' say 'Return 0 on success or a LIBUSB_ERROR code on failure.'
(3) core.c:1163:7: Use of memory after it is freed
This could be a false positive as it's from the static analyzer. It's code path dependant, and really you need the Xcode GUI to follow the flow. Nathan, could you look?

I will take a look at both 2 and 3 today.


-Nathan

[Sean McBride]

Post by Nathan Hjelm
Nope. struct timeval is POSIX (http://pubs.opengroup.org/onlinepubs/
7999959899/basedefs/sys/time.h.html) and always has exactly 2 members.

OK, can someone merge this then:
<https://github.com/seanm/libusbx/commit/29e35526c76801479d158c484ec863615638aee8>

Post by Nathan Hjelm
I will take a look at both 2 and 3 today.

Thanks!

[Sean McBride]

Post by Sean McBride

Post by Sean McBride
(2) os/darwin_usb.c:531:10: Implicit conversion loses integer

precision: 'size_t' (aka 'unsigned long') to 'int'

Post by Sean McBride
A cast would silence this, but actually I worry the last line of the

function is returning entirely the wrong thing. Shouldn't it return
'ret' not 'len'? The docs for 'get_config_descriptor' say 'Return 0 on
success or a LIBUSB_ERROR code on failure.'

Post by Sean McBride
(3) core.c:1163:7: Use of memory after it is freed
This could be a false positive as it's from the static analyzer. It's

code path dependant, and really you need the Xcode GUI to follow the
flow. Nathan, could you look?
I will take a look at both 2 and 3 today.

Nathan,

Just wanted to ping you on these. Both issues still exist in master.

Cheers,

[Nathan Hjelm]

On Jan 21, 2014, at 10:30 AM, Tim Roberts <***@probo.com> wrote:

Nathan Hjelm wrote:
On Jan 20, 2014, at 4:56 PM, Tim Roberts <***@probo.com> wrote:

Even if there isn't, the standard guarantees that those two constructs
are identical. Any elements that don't have initializer values get zeroed.
Not quite. This is true for global/static variables. Anything allocated on the stack that is not explicitly initialized gets whatever was on already the stack.

Sorry, but this is incorrect.  What you say is true if I omit the initializer list completely.  But if I provide a PARTIAL initializer list, then the rest of the structure is zeroed, regardless of the storage class.

C99 Standard 6.7.8.21
If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.
 

Ah. Ok. Wasn't aware of that case. The code is question is ISO C89 to support MSVC. Do you know if the C89 standard also guarantees the rest of the struct is zeroed?

-Nathan

[Tim Roberts]

Post by Nathan Hjelm
Ah. Ok. Wasn't aware of that case. The code is question is ISO C89 to
support MSVC. Do you know if the C89 standard also guarantees the rest
of the struct is zeroed?

Yes. That C99 wording was copied from the C89 spec, section 3.5.7.

[Tim Roberts]

Post by Tim Roberts

Post by Nathan Hjelm
Ah. Ok. Wasn't aware of that case. The code is question is ISO C89 to
support MSVC. Do you know if the C89 standard also guarantees the rest
of the struct is zeroed?

Yes. That C99 wording was copied from the C89 spec, section 3.5.7.

HAVING SAID ALL OF THAT, however, I don't actually have an objection to
the modification. It serves no purpose to the compilation, but if it
satisfies an anal-retentive lint checker and does no damage, that's OK.

[Orin Eman]

I've not met an MSVC compiler that hasn't implemented {0} correctly.

Perhaps it's the comma Xcode doesn't like: {0,} and {0} would be OK.
(Assuming the comma in the original post wasn't a typo.)

I haven't compiled libusb(x) on the Mac since I got forced into installing
Xcode 5... I'll give it a try today if I remember.

Post by Tim Roberts

Post by Tim Roberts

Post by Nathan Hjelm
Ah. Ok. Wasn't aware of that case. The code is question is ISO C89 to
support MSVC. Do you know if the C89 standard also guarantees the rest
of the struct is zeroed?

Yes. That C99 wording was copied from the C89 spec, section 3.5.7.

HAVING SAID ALL OF THAT, however, I don't actually have an objection to
the modification. It serves no purpose to the compilation, but if it
satisfies an anal-retentive lint checker and does no damage, that's OK.
--
Providenza & Boekelheide, Inc.
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
libusbx-devel mailing list
https://lists.sourceforge.net/lists/listinfo/libusbx-devel

[Sean McBride]

Post by Orin Eman
I've not met an MSVC compiler that hasn't implemented {0} correctly.
Perhaps it's the comma Xcode doesn't like: {0,} and {0} would be OK.
(Assuming the comma in the original post wasn't a typo.)

No, the warning applies in both cases.

Guys, it's a two field struct! Nathan (no offense!) was confused by the actual behaviour of {0,} and that's enough proof that {0,0} is simply more readable and less ambiguous. Let's not beat this horse further, someone just merge my 2 character change please. :)

Cheers,

[Pete Batard]

Post by Sean McBride
just merge my 2 character change please. :)

Done for libusbx. Will do the same for libusb later.

Regards,

/Pete

[Tim Roberts]

Post by Orin Eman
Perhaps it's the comma Xcode doesn't like: {0,} and {0} would be OK.
(Assuming the comma in the original post wasn't a typo.)

The trailing comma is specifically allowed by the specs.

I love this language lawyer stuff, and I apologize for starting this
diversion here...

[Nathan Hjelm]

On Mar 21, 2014, at 11:49 AM, Sean McBride <***@rogue-research.com> wrote:

On Tue, 21 Jan 2014 07:43:10 -0700, Nathan Hjelm said:

       >        > (2) os/darwin_usb.c:531:10: Implicit conversion loses integer
       >precision: 'size_t' (aka 'unsigned long') to 'int'
       >        >
       >        > A cast would silence this, but actually I worry the last line of the
       >function is returning entirely the wrong thing. Shouldn't it return
       >'ret' not 'len'? The docs for 'get_config_descriptor' say 'Return 0 on
       >success or a LIBUSB_ERROR code on failure.'
       >        >
       >        >
       >        > (3) core.c:1163:7: Use of memory after it is freed
       >        >
       >        > This could be a false positive as it's from the static analyzer. It's
       >code path dependant, and really you need the Xcode GUI to follow the
       >flow. Nathan, could you look?
       >
       >I will take a look at both 2 and 3 today.

Nathan,

Just wanted to ping you on these. Both issues still exist in master.
 
I can't get scan-build to give me the the first one but I do get the second one. The second one is an interesting flow and will take some thought to see if it identifies a real issue or not. clang assumes that the call to libusb_unref_device in discovered_devs_free will result in a call to free on a device that is being returned. I am not convinced that can ever be the case.

-Nathan

[Sean McBride]

Post by Nathan Hjelm
I can't get scan-build to give me the the first one

It's a compiler warning, not a static analyzer warning. I get it just building with Xcode 5.0.2. Again, I think last line of the function is returning entirely the wrong thing. Shouldn't it return 'ret' not 'len'? The docs for 'get_config_descriptor' say 'Return 0 on success or a LIBUSB_ERROR code on failure.'

Post by Nathan Hjelm
but I do get the
second one. The second one is an interesting flow and will take some
thought to see if it identifies a real issue or not.

Agreed, it's not at all clear to me either. The analyzer sometimes catches amazing things and sometimes is amazingly wrong. Thanks for looking into it.

Cheers,

[Nathan Hjelm]

On Mar 21, 2014, at 12:44 PM, Nathan Hjelm <***@me.com> wrote:



On Mar 21, 2014, at 11:49 AM, Sean McBride <***@rogue-research.com> wrote:

On Tue, 21 Jan 2014 07:43:10 -0700, Nathan Hjelm said:

       >        > (2) os/darwin_usb.c:531:10: Implicit conversion loses integer
       >precision: 'size_t' (aka 'unsigned long') to 'int'
       >        >
       >        > A cast would silence this, but actually I worry the last line of the
       >function is returning entirely the wrong thing. Shouldn't it return
       >'ret' not 'len'? The docs for 'get_config_descriptor' say 'Return 0 on
       >success or a LIBUSB_ERROR code on failure.'
       >        >
       >        >
       >        > (3) core.c:1163:7: Use of memory after it is freed
       >        >
       >        > This could be a false positive as it's from the static analyzer. It's
       >code path dependant, and really you need the Xcode GUI to follow the
       >flow. Nathan, could you look?
       >
       >I will take a look at both 2 and 3 today.

Nathan,

Just wanted to ping you on these. Both issues still exist in master.
 
I can't get scan-build to give me the the first one but I do get the second one. The second one is an interesting flow and will take some thought to see if it identifies a real issue or not. clang assumes that the call to libusb_unref_device in discovered_devs_free will result in a call to free on a device that is being returned. I am not convinced that can ever be the case.
 
Hmm, looking closer there might be a race condition in here. In the case that we have hotplug the device could get released between the generation of the discovered devices array and the call to reference the device. If this is a race then the window is very small.

-Nathan

[Nathan Hjelm]

On Mar 21, 2014, at 12:50 PM, Sean McBride <***@rogue-research.com> wrote:

On Fri, 21 Mar 2014 18:44:01 +0000, Nathan Hjelm said:

       >I can't get scan-build to give me the the first one

It's a compiler warning, not a static analyzer warning. I get it just building with Xcode 5.0.2. Again, I think last line of the function is returning entirely the wrong thing. Shouldn't it return 'ret' not 'len'? The docs for 'get_config_descriptor' say 'Return 0 on success or a LIBUSB_ERROR code on failure.'
 
We should probably update the documentation for the internal call. Both the darwin and linux backends return the length actually read. The code calling get_config_descriptor also assumes the returned value is the length. I will push an update to the documentation and fix the warning.


       >but I do get the
       >second one. The second one is an interesting flow and will take some
       >thought to see if it identifies a real issue or not.

Agreed, it's not at all clear to me either. The analyzer sometimes catches amazing things and sometimes is amazingly wrong. Thanks for looking into it.
 
No problem. I hope to have a determination on whether this is a bug later today.

-Nathan