diff mbox

add attribute nonstring

Message ID 1510701753.4087.24.camel@cavium.com
State New, archived
Headers

Commit Message

Steve Ellcey Nov. 14, 2017, 11:22 p.m. UTC 
  On Tue, 2017-11-14 at 19:29 +0100, Florian Weimer wrote:
> 
> So the only thing that's missing is the __set_errno (ENODEV); call, I 
> think.  (It's what the ioctl should fail with for an unknown interface 
> name.)
> 
> Thanks,
> Florian

OK, here is a new version of the patch that sets errno to ENODEV.  I
tested it (Thanks to Joseph for fixing the tests that would not compile
due to the new GCC warning) and got three failures that I think are all
unrelated to this change:

FAIL: crypt/badsalttest
FAIL: nptl/tst-thread_local1
FAIL: nss/tst-nss-files-hosts-multi

tst-thread_local1 is a failure I have seen before, tst-nss-files-hosts-
multi is a new test, and I am not sure what is happening with
badsalttest but it does not seem to be related to this change.  I also
have the not-checked-in __NONSTRING changes in utmp.h in my tree in
order to get this to build.

OK to checkin?

Steve Ellcey

2017-11-14  Steve Ellcey  <sellcey@cavium.com>

	* sysdeps/unix/sysv/linux/if_index.c (__if_nametoindex):
	Check if ifname is too long.

Comments

Florian Weimer Nov. 15, 2017, 10:48 a.m. UTC | #1 
On 11/15/2017 12:22 AM, Steve Ellcey wrote:
> On Tue, 2017-11-14 at 19:29 +0100, Florian Weimer wrote:
>>
>> So the only thing that's missing is the __set_errno (ENODEV); call, I
>> think.  (It's what the ioctl should fail with for an unknown interface
>> name.)
>>
>> Thanks,
>> Florian
> 
> OK, here is a new version of the patch that sets errno to ENODEV.  I
> tested it (Thanks to Joseph for fixing the tests that would not compile
> due to the new GCC warning) and got three failures that I think are all
> unrelated to this change:
> 
> FAIL: crypt/badsalttest
> FAIL: nptl/tst-thread_local1
> FAIL: nss/tst-nss-files-hosts-multi

Can you provide more details (such as .out file contents)?  Even if 
these failures are unrelated, they might point to something we need to fix.

> tst-thread_local1 is a failure I have seen before, tst-nss-files-hosts-
> multi is a new test, and I am not sure what is happening with
> badsalttest but it does not seem to be related to this change.  I also
> have the not-checked-in __NONSTRING changes in utmp.h in my tree in
> order to get this to build.
> 
> OK to checkin?

Patch looks good to me.  Please add [BZ #22442] to both the ChangeLog 
entry and the commit message because it is a user-visible bug.

Thanks,
Florian
Steve Ellcey Nov. 15, 2017, 5:58 p.m. UTC | #2 
On Wed, 2017-11-15 at 11:48 +0100, Florian Weimer wrote:
> 
> > FAIL: crypt/badsalttest
> > FAIL: nptl/tst-thread_local1
> > FAIL: nss/tst-nss-files-hosts-multi
> Can you provide more details (such as .out file contents)?  Even if 
> these failures are unrelated, they might point to something we need
> to fix.

bassalttest.out:

Didn't expect signal from child: got `Segmentation fault'


tst-thread_local1.out is empty, tst-thread_local1.test-result has:

FAIL: nptl/tst-thread_local1
original exit status 1

tst-nss-files-hosts-multi.out:

Timed out: killed the child process



Steve Ellcey
sellcey@cavium.com
Adhemerval Zanella Nov. 16, 2017, 12:37 a.m. UTC | #3 
On 15/11/2017 15:58, Steve Ellcey wrote:
> On Wed, 2017-11-15 at 11:48 +0100, Florian Weimer wrote:
>>  
>>> FAIL: crypt/badsalttest
>>> FAIL: nptl/tst-thread_local1
>>> FAIL: nss/tst-nss-files-hosts-multi
>> Can you provide more details (such as .out file contents)?  Even if 
>> these failures are unrelated, they might point to something we need
>> to fix.
> 
> bassalttest.out:
> 
> Didn't expect signal from child: got `Segmentation fault'

This is something we should investigate.

> 
> 
> tst-thread_local1.out is empty, tst-thread_local1.test-result has:
> 
> FAIL: nptl/tst-thread_local1
> original exit status 1

I usually see it when I try with a toolchain that requires a GLIBCXX_*
version higher than then one installed on the system (for instance
the one generated by build-many-glibcs.py). The test should ran without
issues if you pass the path of built libstdc++ with the usual paths
in testrun.sh.

> 
> tst-nss-files-hosts-multi.out:
> 
> Timed out: killed the child process
> 
> 
> 
> Steve Ellcey
> sellcey@cavium.com
>
Szabolcs Nagy Nov. 17, 2017, 12:49 p.m. UTC | #4 
On 15/11/17 17:58, Steve Ellcey wrote:
> tst-nss-files-hosts-multi.out:
> 
> Timed out: killed the child process
> 

i've seen this timeout too (in virtual machine),
it does 1.5M syscalls a third of which is slow socket syscalls.

i think that's a bit too much, less iterations should be enough.
Florian Weimer Nov. 17, 2017, 12:56 p.m. UTC | #5 
On 11/17/2017 01:49 PM, Szabolcs Nagy wrote:
> On 15/11/17 17:58, Steve Ellcey wrote:
>> tst-nss-files-hosts-multi.out:
>>
>> Timed out: killed the child process
>>
> 
> i've seen this timeout too (in virtual machine),
> it does 1.5M syscalls a third of which is slow socket syscalls.
> 
> i think that's a bit too much, less iterations should be enough.

The buffer code in nss_files (and nss_db) is really sensitive to input 
sizes, and we have seen bugs which are triggered with very specific 
result sizes only, hence the the first loop with the small counts.

The 22222 count is arbitrary, but a fairly large value is needed to make 
the super-linear behavior of the old implementation immediately obvious. 
  An alternative would be to reduce the RLIMIT_AS limit considerably, 
but that might unduly affect architectures which are wasteful with 
address space.

Thanks,
Florian
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/if_index.c b/sysdeps/unix/sysv/linux/if_index.c
index 56f3f13..e7ca27b 100644
--- a/sysdeps/unix/sysv/linux/if_index.c
+++ b/sysdeps/unix/sysv/linux/if_index.c
@@ -43,6 +43,12 @@  __if_nametoindex (const char *ifname)
   if (fd < 0)
     return 0;
 
+  if (strlen (ifname) >= IFNAMSIZ)
+    {
+      __set_errno (ENODEV);
+      return 0;
+    }
+
   strncpy (ifr.ifr_name, ifname, sizeof (ifr.ifr_name));
   if (__ioctl (fd, SIOCGIFINDEX, &ifr) < 0)
     {