Re: CVS commit: src/usr.bin/grep

[enami tsugutomo]

Joerg Sonnenberger jo...@netbsd.org writes:

 Module Name:  src
 Committed By: joerg
 Date: Wed Feb 16 01:31:34 UTC 2011
 
 Modified Files:
   src/usr.bin/grep: Makefile file.c grep.1 grep.c grep.h queue.c util.c
 Added Files:
   src/usr.bin/grep: fastgrep.c

I just did `more fastgrep.c' and found following piece of code.  The
usage of wflag is an obvious regression from OpenBSD code.

|   if (fg-len = 14 
|   strncmp(pat + (fg-bol ? 1 : 0), [[::]], 7) == 0 
|   strncmp(pat + (fg-bol ? 1 : 0) + fg-len - 7, [[::]], 7) == 0) {
|   fg-len -= 14;
|   /* Word boundary is handled separately in util.c */
|   wflag = true;
|   }
|
|   /*
|* Copy pattern minus '^' and '$' characters as well as word
|* match character classes at the beginning and ending of the
|* string respectively.
|*/
|   fg-pattern = grep_malloc(fg-len + 1);
|   strlcpy((char *)fg-pattern, pat + (bol ? 1 : 0) + wflag,
|   fg-len + 1);

enami.

Re: CVS commit: src/usr.bin/grep

[enami tsugutomo]

 In which sense?

The meaning of `... + wflag' was to skip 7 chars.

enami.

Re: CVS commit: src/sys/arch/i386/i386

[enami tsugutomo]

=?UTF-8?B?R3LDqWdvaXJlIFN1dHJl?= gsu...@netbsd.org writes:

 Assume for instance that the boot-loader left us with:
 
   +--+   ++ +--+
   | string table |   | kernel | | symbol table |
   +--+   ++ +--+
 
 The new addresses computed by lines 338-359 (here, it's really
 lines 344-345) will move the tables so that they end up as:
 
  ++--+--+
  | kernel | symbol table | string table |
  ++--+--+

If this ascii art is correct, memmove should be used instead of
memcpy.  Also, if initial order is kernel, string table, symbol table
and a gap between kernel and string table is smaller than symbol
table, copying it may overwrite the string table.

Is there any guarantee that the gap is large enough?

enami.

Re: CVS commit: src/usr.bin/pkill

[enami tsugutomo]

 cvs rdiff -u -r1.25 -r1.26 src/usr.bin/pkill/pkill.c

Looks like -l option is necessary for prenice also.

enami.

Re: CVS commit: src/lib/libc/stdlib

[enami tsugutomo]

 Now the code disagrees with the comment above it.  Please fix the comment.

I've changed the comment but feel free to improve it if you have
better one.

enami.

Re: CVS commit: src/sys/netinet

[enami tsugutomo]

 Yes, this patch makes boot with root on nfs work again.
 Please commit.

Just committed.

enami.

Re: CVS commit: src/sys/netinet

[enami tsugutomo]

  Can you try with a LOCKDEBUG + DEBUG/DIAGNOSTIC kernel?
 
 The kernel has all three options.

Hm, if you have DEBUG enabled, kern_free() might fill free'ed area
with WEIRD_ADDR.

enami.

Index: sys/netinet/ip_reass.c
===
RCS file: /cvsroot/src/sys/netinet/ip_reass.c,v
retrieving revision 1.4
diff -u -r1.4 ip_reass.c
--- sys/netinet/ip_reass.c  3 Oct 2010 19:44:47 -   1.4
+++ sys/netinet/ip_reass.c  6 Oct 2010 01:52:14 -
@@ -390,7 +390,6 @@
pool_cache_put(ipfren_cache, q);
m_cat(m, t);
}
-   free(fp, M_FTABLE);
 
/*
 * Create header for new packet by modifying header of first
@@ -400,6 +399,7 @@
ip-ip_len = htons((ip-ip_hl  2) + next);
ip-ip_src = fp-ipq_src;
ip-ip_dst = fp-ipq_dst;
+   free(fp, M_FTABLE);
 
m-m_len += (ip-ip_hl  2);
m-m_data -= (ip-ip_hl  2);

Re: CVS commit: src/lib/libc/stdlib

[enami tsugutomo]

In addition to previous, __allocenv() call in unsetenv() isn't
necessary if we check if there exists the bitmap before touching it.
If still want to call it, it must be protected by the lock.

enami.

Re: CVS commit: src/lib/libc/stdlib

[enami tsugutomo]

 Module Name:  src
 Committed By: christos
 Date: Thu Sep 23 16:02:41 UTC 2010
 
 Modified Files:
   src/lib/libc/stdlib: setenv.c
 
 Log Message:
 PR/43899: Nicolas Joly: setenv(3)/unsetenv(3) memory leak.
 Partial fix: Don't allocate a new string if the length is equal to the
 old length, because presumably the old string was also nul terminated
 so it has the extra byte needed.
 The real fix is to keep an adjunct array of bits, one for each environmen=
 t
 variable and keep track if the entry was allocated or not so that we can
 free it in unsetenv.
 
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.32 -r1.33 src/lib/libc/stdlib/setenv.c

Actual change done in setenv.c rev. 1.33 causes following program
memory leak (while rev. 1.32 does not).

#include stdlib.h

main()
{

while (1)
setenv(dummyvar, dummyval, 1);
}

And rev. 1.34 introduces lock leak on error path while it leaves
another fixable memory leak.

I guess following patch is worth to be applied.

enami.

Index: lib/libc/stdlib/setenv.c
===
RCS file: /cvsroot/src/lib/libc/stdlib/setenv.c,v
retrieving revision 1.34
diff -u -r1.34 setenv.c
--- lib/libc/stdlib/setenv.c23 Sep 2010 17:30:49 -  1.34
+++ lib/libc/stdlib/setenv.c24 Sep 2010 03:43:19 -
@@ -81,7 +81,7 @@
c = __findenv(name, offset);
 
if (__allocenv(offset) == -1)
-   return -1;
+   goto bad;
 
if (*value == '=')  /* no `=' in value */
++value;
@@ -90,7 +90,7 @@
if (c != NULL) {
if (!rewrite)
goto good;
-   if (strlen(c)  l_value)/* old larger; copy over */
+   if (strlen(c) = l_value)   /* old is enough; copy over */
goto copy;
} else {/* create new slot */
size = (size_t)(sizeof(char *) * (offset + 2));
@@ -113,6 +113,8 @@
/* name + `=' + value */
if ((c = malloc(size + l_value + 2)) == NULL)
goto bad;
+   if (bit_test(__environ_malloced, offset))
+   free(environ[offset]);
environ[offset] = c;
(void)memcpy(c, name, size);
c += size;

Re: CVS commit: src/sys/kern

[enami tsugutomo]

 -wrong initialization reported in a followup to PR bin/43336
  (looks harmless because it applies to zero-initialized memory, so
  LIST_INIT() is a no-op)

Does malloc(3) return zero-initialized memory?

enami.

Re: CVS commit: src/sys

[enami tsugutomo]

 cvs rdiff -u -r1.11 -r1.12 src/sys/kern/subr_xcall.c

Why XC_PRI_BIT is not placed at LSB instead MSB?  Is cpu state change
(from offline to online or vise versa) cared?

enami.

Re: CVS commit: src/sys

[enami tsugutomo]

 enami tsugutomo tsugutomo.en...@jp.sony.com wrote:
   cvs rdiff -u -r1.11 -r1.12 src/sys/kern/subr_xcall.c
  
  Why XC_PRI_BIT is not placed at LSB instead MSB?
 
 Because you would lose the value (note, it is not a pointer).

A flag bit at LSB can be preserved by incrementing the counter by 2,
can't it?

Subtraction should be used to care counter wrap around that makes it
easier.

enami.

Re: CVS commit: src/lib/libpthread

[enami tsugutomo]

 Modified Files:
   src/lib/libpthread: pthread.c
 
 Log Message:
 fix the pthread pt_lid in the fork callback function that runs in the child=
  instead of a function that may be going away.  KNFify
 
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.114 -r1.115 src/lib/libpthread/pthread.c

The _lwp_ctl() call also need to be called with self-pt_lwpctl
doesn't it?

enami.

@@ -235,11 +235,14 @@
 static void
 pthread__fork_callback(void)
 {
+   struct __pthread_st *self;
 
/* lwpctl state is not copied across fork. */
if (_lwp_ctl(LWPCTL_FEATURE_CURCPU, pthread__first-pt_lwpctl)) {
err(1, _lwp_ctl);
}
+   self = pthread__self();
+   self-pt_lid = _lwp_self();
 }
 
 static void

Re: CVS commit: src/sys/dev/usb

[enami tsugutomo]

  Modified Files:
  src/sys/dev/usb: uyurex.c
 
  Log Message:
  The monitor struct member is gone.  Make this compile again.
 
 Hmmm, wonder how I missed that one.  Thanks for catching.

Never mind.  Since uyurex(4) isn't enabled on any kernel config file,
it's not you fault.

enami.

CVS commit: src/lib/libc/stdlib

[enami tsugutomo]

Module Name:src
Committed By:   enami
Date:   Thu Mar  4 22:48:31 UTC 2010

Modified Files:
src/lib/libc/stdlib: jemalloc.c

Log Message:
Fix race condition on reallocation of huge category.

We need to remove the old region before mremap() since if it relesae the
old region, other thread may map it for the same huge category allocation
and insert it to the tree before we acquire a lock after mremap().

Fixes PR/42876.


To generate a diff of this commit:
cvs rdiff -u -r1.20 -r1.21 src/lib/libc/stdlib/jemalloc.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

CVS commit: src/usr.bin/sort

[enami tsugutomo]

Module Name:src
Committed By:   enami
Date:   Fri Feb  5 21:58:42 UTC 2010

Modified Files:
src/usr.bin/sort: fsort.c msort.c sort.c sort.h

Log Message:
Don't touch past the end of allocated region.  It results segmentation
violation.


To generate a diff of this commit:
cvs rdiff -u -r1.46 -r1.47 src/usr.bin/sort/fsort.c
cvs rdiff -u -r1.29 -r1.30 src/usr.bin/sort/msort.c
cvs rdiff -u -r1.57 -r1.58 src/usr.bin/sort/sort.c
cvs rdiff -u -r1.30 -r1.31 src/usr.bin/sort/sort.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Re: CVS commit: [matt-nb5-mips64] src/sys/uvm

[enami tsugutomo]

  To generate a diff of this commit:
  cvs rdiff -u -r1.42 -r1.42.16.1 src/sys/uvm/uvm_pglist.c
 
/*
 * Test both the ending and starting pages to see if =
 they are
 * both free.  If the ending and starting pages are =
 same page,
 * we only test one of them.  If the pages aren't free, =
 there
 * is no reason to continue this iteration so advance =
 to the
 * next address and try again.
 */
if (VM_PAGE_IS_FREE(pgs[end - 1]) == 0
|| end - 1 == tryidx + skip
|| VM_PAGE_IS_FREE(pgs[tryidx + skip]) == 0) {
try += roundup(num, align);
skip = 0;
continue;
}
 
  I guess this part can be improved and/or fixed as below.
 
 I initially did something like that but realized it didn't really buy me
 anything since the compiler will actually do something like that
 anyways.

Ah, you're missing my point.  Let me rephrase again.

1) If `VM_PAGE_IS_FREE(pgs[end - 1]) == 0' is false and `end - 1 ==
   tryidx + skip' is true, above code skips the region but actually we
   found the enough space.

2) If `VM_PAGE_IS_FREE(pgs[end - 1]) == 0' is false and `end - 1 ==
   tryidx + skip' is false but `VM_PAGE_IS_FREE(pgs[tryidx + skip])
   == 0' is true, it is better to advance the `try' by roundup(skip +
   1, align) instead of roundup(num, align).

enami.

Re: CVS commit: src/tools

[enami tsugutomo]

matthew green m...@netbsd.org writes:

 Module Name:  src
 Committed By: mrg
 Date: Mon Dec 21 20:57:36 UTC 2009
 
 Modified Files:
   src/tools: Makefile
 
 Log Message:
 move the build of pax before libelf.  fixes my build of tools/libelf,
 though i didn't look to see why libelf needs pax.
 
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.130 -r1.131 src/tools/Makefile

Index: tools/Makefile
===
RCS file: /cvsroot/src/tools/Makefile,v
retrieving revision 1.130
retrieving revision 1.131
diff -u -r1.130 -r1.131
--- tools/Makefile21 Dec 2009 18:21:17 -1.130
+++ tools/Makefile21 Dec 2009 20:57:36 -1.131
@@ -43,12 +43,12 @@
 yacc .WAIT \
 awk .WAIT \
 lex .WAIT \
+pax .WAIT \
 libelf .WAIT \
 ${TOOLCHAIN_BITS} \
 asn1_compile atf-compile cat cksum compile_et config db \
 file lint1 \
 makefs menuc mkcsmapper mkesdb mklocale mknod msgc \
-pax .WAIT \
 disklabel .WAIT \
 paxctl .WAIT \
 fdisk .WAIT \

I guess this may break parallel build.  Before, building disklabel is
deferred but now it will be built more earlier.

The notation like below

xxx .WAIT \
yyy .WAIT \
zzz ...

is quite misleading and it should be rewritten as follows instead:

xxx \
.WAIT yyy \
.WAIT zzz ...

enami.

Re: CVS commit: src/tools/gdb

[enami tsugutomo]

 Index: gnu/dist/gdb6/sim/mips/Makefile.in
 ===
 RCS file: /cvsroot/src/gnu/dist/gdb6/sim/mips/Makefile.in,v
 retrieving revision 1.1.1.2
 diff -u -r1.1.1.2 Makefile.in
 --- gnu/dist/gdb6/sim/mips/Makefile.in2 Jul 2006 20:28:34 -   
 1.1.1.2
 +++ gnu/dist/gdb6/sim/mips/Makefile.in15 Dec 2009 07:33:37 -
 @@ -84,7 +84,7 @@
  multi-run.o: multi-include.h tmp-mach-multi
  
  ../igen/igen:
 - cd ../igen  $(MAKE)
 + (cd ../igen  $(MAKE))
  
  IGEN_TRACE= # -G omit-line-numbers # -G trace-rule-selection -G 
 trace-rule-rejection -G trace-entries # -G trace-all
  IGEN_INSN=$(srcdir)/mips.igen
 @@ -132,7 +132,7 @@
  $(BUILT_SRC_FROM_IGEN): tmp-igen
  
  tmp-igen: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
 - cd ../igen  $(MAKE)
 + (cd ../igen  $(MAKE))
   ../igen/igen \
   $(IGEN_TRACE) \
   -I $(srcdir) \

Hm, it may be simpler to remove `cd ../igen  $(MAKE)' except the
../igen/igen target.

enami.

Re: CVS commit: src/gnu/dist/gdb6/gdb

[enami tsugutomo]

 The above is defining a global symbol that referes to the middle of
 a struct, not setting a memory location to the address of the it.

Do you mean lwp0.l_addr may vary during execution?

enami.

Re: CVS commit: src/gnu/dist/gdb6/gdb

[enami tsugutomo]

 Module Name:  src
 Committed By: enami
 Date: Fri Nov 27 02:51:15 UTC 2009
 
 Modified Files:
   src/gnu/dist/gdb6/gdb: bsd-kvm.c
 
 Log Message:
 Lookup lwp0.l_addr instead of proc0paddr to locate PCB.

I wonder if it is better to keep proc0paddr in kernel as a pointer to
PCB rather than embedding struct offset in gdb binary.

enami.

Re: CVS commit: src/sys/kern

[enami tsugutomo]

 Log Message:
 Move common logic in selcommon() and pollcommon() into sel_do_scan().
 Avoids code duplication.  XXX: pollsock() should be converted too, except
 it's a bit ugly.
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.16 -r1.17 src/sys/kern/sys_select.c

The test `if (selpoll)' is unnecessary if scanner function is passed
directly instead of the flag.

-   error = selscan(l, (fd_mask *)(bits + ni * 0),
-   (fd_mask *)(bits + ni * 3), nd, retval);
+   if (selpoll) {
+   error = selscan((char *)fds, nfds, retval);
+   } else {
+   error = pollscan((struct pollfd *)fds, nfds, retval);
+   }
 
enami.