Re: ufs free()

2018-03-31 Thread David Hill
On Sat, Mar 31, 2018 at 11:15:35PM +0200, Christian Weisgerber wrote:
> David Hill:
> 
> > This diff adds sizes to free(), which completes ufs/ffs.
> 
> It's broken at least for softdep+UFS2.  This chunk blows up:
> 
> > --- ufs/ffs/ffs_softdep.c   10 Feb 2018 05:24:23 -  1.138
> > +++ ufs/ffs/ffs_softdep.c   29 Mar 2018 02:55:37 -
> > @@ -4034,7 +4036,8 @@ handle_written_inodeblock(struct inodede
> > *dp1 = *inodedep->id_savedino1;
> > else
> > *dp2 = *inodedep->id_savedino2;
> > -   free(inodedep->id_savedino1, M_INODEDEP, 0);
> > +   free(inodedep->id_savedino1, M_INODEDEP,
> > +   sizeof(struct ufs1_dinode));
> > inodedep->id_savedino1 = NULL;
> > if ((bp->b_flags & B_DELWRI) == 0)
> > stat_inode_bitmap++;
> 
> panic: free: size too small 128 <= 256 / 2 (0x8099a700) type inodedep
> Stopped at  db_enter+0x5:   popq%rbp
> TIDPIDUID PRFLAGS PFLAGS  CPU  COMMAND
> db_enter() at db_enter+0x5
> panic() at panic+0x129
> free(ff07259d97c8,ff07259d97c8,ff071c6d4ef8) at free+0x35b
> handle_written_inodeblock(ff07259d97c8,ff071c6d4e00) at 
> handle_written_
> inodeblock+0x1b5
> softdep_disk_write_complete(ff071c6d4e00) at 
> softdep_disk_write_complete+0x
> 255
> biodone(ff071bd999e8) at biodone+0x73
> 
> id_savedino1 and id_savedino2 are a union, so it's pretty clear
> that the free() needs to distinguish between the UFS1 and UFS2 cases
> like the preceding lines do.
> 
> I don't know if check_inode_unwritten() needs to make the same
> distinction:
> 
> > @@ -2307,7 +2307,8 @@ check_inode_unwritten(struct inodedep *i
> > if (inodedep->id_state & ONWORKLIST)
> > WORKLIST_REMOVE(>id_list);
> > if (inodedep->id_savedino1 != NULL) {
> > -   free(inodedep->id_savedino1, M_INODEDEP, 0);
> > +   free(inodedep->id_savedino1, M_INODEDEP,
> > +   sizeof(struct ufs1_dinode));
> > inodedep->id_savedino1 = NULL;
> > }
> > if (free_inodedep(inodedep) == 0) {
> 
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
>

Can you try this diff?

Index: ufs/ffs/ffs_softdep.c
===
RCS file: /cvs/src/sys/ufs/ffs/ffs_softdep.c,v
retrieving revision 1.139
diff -u -p -r1.139 ffs_softdep.c
--- ufs/ffs/ffs_softdep.c   30 Mar 2018 17:35:20 -  1.139
+++ ufs/ffs/ffs_softdep.c   1 Apr 2018 00:55:10 -
@@ -2291,6 +2291,8 @@ softdep_freefile(struct vnode *pvp, ufsi
 STATIC int
 check_inode_unwritten(struct inodedep *inodedep)
 {
+   size_t freesize;
+
splassert(IPL_BIO);
 
if ((inodedep->id_state & DEPCOMPLETE) != 0 ||
@@ -2307,8 +2309,11 @@ check_inode_unwritten(struct inodedep *i
if (inodedep->id_state & ONWORKLIST)
WORKLIST_REMOVE(>id_list);
if (inodedep->id_savedino1 != NULL) {
-   free(inodedep->id_savedino1, M_INODEDEP,
-   sizeof(struct ufs1_dinode));
+   if (inodedep->id_fs->fs_magic == FS_UFS1_MAGIC)
+   freesize = sizeof(struct ufs1_dinode);
+   else
+   freesize = sizeof(struct ufs2_dinode);
+   free(inodedep->id_savedino1, M_INODEDEP, freesize);
inodedep->id_savedino1 = NULL;
}
if (free_inodedep(inodedep) == 0) {
@@ -4006,6 +4011,7 @@ handle_written_inodeblock(struct inodede
struct ufs1_dinode *dp1 = NULL;
struct ufs2_dinode *dp2 = NULL;
int hadchanges, fstype;
+   size_t freesize;
 
splassert(IPL_BIO);
 
@@ -4015,10 +4021,12 @@ handle_written_inodeblock(struct inodede
 
if (inodedep->id_fs->fs_magic == FS_UFS1_MAGIC) {
fstype = UM_UFS1;
+   freesize = sizeof(struct ufs1_dinode);
dp1 = (struct ufs1_dinode *) bp->b_data +
ino_to_fsbo(inodedep->id_fs, inodedep->id_ino);
} else {
fstype = UM_UFS2;
+   freesize = sizeof(struct ufs2_dinode);
dp2 = (struct ufs2_dinode *) bp->b_data +
ino_to_fsbo(inodedep->id_fs, inodedep->id_ino);
}
@@ -4035,8 +4043,7 @@ handle_written_inodeblock(struct inodede
*dp1 = *inodedep->id_savedino1;
else
*dp2 = *inodedep->id_savedino2;
-   free(inodedep->id_savedino1, M_INODEDEP,
-   sizeof(struct ufs1_dinode));
+   free(inodedep->id_savedino1, M_INODEDEP, freesize);
inodedep->id_savedino1 = NULL;
if ((bp->b_flags & B_DELWRI) == 0)
stat_inode_bitmap++;



Re: ufs free()

2018-03-31 Thread Christian Weisgerber
David Hill:

> This diff adds sizes to free(), which completes ufs/ffs.

It's broken at least for softdep+UFS2.  This chunk blows up:

> --- ufs/ffs/ffs_softdep.c 10 Feb 2018 05:24:23 -  1.138
> +++ ufs/ffs/ffs_softdep.c 29 Mar 2018 02:55:37 -
> @@ -4034,7 +4036,8 @@ handle_written_inodeblock(struct inodede
>   *dp1 = *inodedep->id_savedino1;
>   else
>   *dp2 = *inodedep->id_savedino2;
> - free(inodedep->id_savedino1, M_INODEDEP, 0);
> + free(inodedep->id_savedino1, M_INODEDEP,
> + sizeof(struct ufs1_dinode));
>   inodedep->id_savedino1 = NULL;
>   if ((bp->b_flags & B_DELWRI) == 0)
>   stat_inode_bitmap++;

panic: free: size too small 128 <= 256 / 2 (0x8099a700) type inodedep
Stopped at  db_enter+0x5:   popq%rbp
TIDPIDUID PRFLAGS PFLAGS  CPU  COMMAND
db_enter() at db_enter+0x5
panic() at panic+0x129
free(ff07259d97c8,ff07259d97c8,ff071c6d4ef8) at free+0x35b
handle_written_inodeblock(ff07259d97c8,ff071c6d4e00) at handle_written_
inodeblock+0x1b5
softdep_disk_write_complete(ff071c6d4e00) at softdep_disk_write_complete+0x
255
biodone(ff071bd999e8) at biodone+0x73

id_savedino1 and id_savedino2 are a union, so it's pretty clear
that the free() needs to distinguish between the UFS1 and UFS2 cases
like the preceding lines do.

I don't know if check_inode_unwritten() needs to make the same
distinction:

> @@ -2307,7 +2307,8 @@ check_inode_unwritten(struct inodedep *i
>   if (inodedep->id_state & ONWORKLIST)
>   WORKLIST_REMOVE(>id_list);
>   if (inodedep->id_savedino1 != NULL) {
> - free(inodedep->id_savedino1, M_INODEDEP, 0);
> + free(inodedep->id_savedino1, M_INODEDEP,
> + sizeof(struct ufs1_dinode));
>   inodedep->id_savedino1 = NULL;
>   }
>   if (free_inodedep(inodedep) == 0) {

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



Re: manpage text width

2018-03-31 Thread Walter Alejandro Iglesias
Hi Ingo,

In article <20180329235743.ge59...@athene.usta.de> Ingo Schwarze 
 wrote:
> > Can I do anything to fix this?
> 
> Yes.
> 

I've always wondered why groff did that nonsense with man pages.  I
remember discussing this same issue in groff mailing lists years ago
(with E. Raymond).

In my opinion, thank you Ingo or whoever had the good idea of not doing
the same with mandoc.


Walter



Re: binutils: build with LLVM 6.0.0

2018-03-31 Thread Mark Kettenis
> Date: Sat, 31 Mar 2018 21:58:06 +0200
> From: Patrick Wildt 
> 
> On Thu, Mar 15, 2018 at 03:55:25PM -0700, William Ahern wrote:
> > On Thu, Mar 15, 2018 at 05:23:24PM +0100, Patrick Wildt wrote:
> > > Hi,
> > > 
> > > LLVM 6.0.0 does now complain of code does computation on NULL pointers,
> > > which apparently binutils makes use of.  I think we can teach binutils
> > > to stop doing that.
> > > 
> > > Is my C foo correct?  Feedback?
> > 
> > Both (type *)0 - 1 and (type *)-1 rely on undefined behavior, but
> > _different_ undefined behavior. Even presuming the former was reliable, is
> > the latter?
> > 
> > FWIW, the two expressions also evaluate to different values,
> > 0xfffc vs 0x. That's not a problem by itself but
> > suggests possible signedness (or whatever you call the analogous issue for
> > pointer arithmetic) pitfalls.
> 
> You're right.  It does have a different value.  On the other hand, the
> code is only used in three places.  One place checks for NULL, the other
> two places are part of the diff.  And the comment above _bfd_elf_archive
> _symbol_lookup actually says:
> 
> /* Return the linker hash table entry of a symbol that might be
>satisfied by an archive symbol.  Return -1 on error.  */
> 
> > Maybe it's safer to use the address of abfd, which should be shared even if
> > archive_symbol_lookup is a function pointer to an external instantiation? I
> > originally thought to use a static global singleton, but the macros and
> > function pointers made me doubt the caller and callee are always from the
> > same object. Using abfd is simpler and I don't see where abfd might be
> > wrapped or proxied.
> 
> I don't understand what you're saying, I don't know the binutils code
> well enough for that.

FWIW, FreeBSD fixed it in a different way:

  https://reviews.freebsd.org/D12928

But upstream did it your way, at least for the elflink.c.  So the
elflink.c bit is ok kettenis@

I'm not sure about the obstack.h bit; why does it complain about
__INT_TO_PTR but not about __PTR_TO_INT?

Maybe we should just disable the warning in question for this crufty
code?  

> > > diff --git a/gnu/usr.bin/binutils-2.17/bfd/elflink.c 
> > > b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > > index a6d17dcb4d9..aa010f9d0b2 100644
> > > --- a/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > > +++ b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > > @@ -4517,7 +4517,7 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
> > >len = strlen (name);
> > >copy = bfd_alloc (abfd, len);
> > >if (copy == NULL)
> > > -return (struct elf_link_hash_entry *) 0 - 1;
> > > +return (struct elf_link_hash_entry *)-1;
> > >  
> > >first = p - name + 1;
> > >memcpy (copy, name, first);
> > > @@ -4629,7 +4629,7 @@ elf_link_add_archive_symbols (bfd *abfd, struct 
> > > bfd_link_info *info)
> > >   }
> > >  
> > > h = archive_symbol_lookup (abfd, info, symdef->name);
> > > -   if (h == (struct elf_link_hash_entry *) 0 - 1)
> > > +   if (h == (struct elf_link_hash_entry *)-1)
> > >   goto error_return;
> > >  
> > > if (h == NULL)
> > > diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h 
> > > b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > > index 88c2a264adc..8839c48e95f 100644
> > > --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> > > +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > > @@ -123,7 +123,7 @@ extern "C" {
> > >  #endif
> > >  
> > >  #ifndef __INT_TO_PTR
> > > -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> > > +# define __INT_TO_PTR(P) ((char *)(P))
> > >  #endif
> > >  
> > >  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is
> 
> 



Re: binutils: build with LLVM 6.0.0

2018-03-31 Thread Patrick Wildt
On Thu, Mar 15, 2018 at 09:31:13PM -0600, Todd C. Miller wrote:
> On Thu, 15 Mar 2018 17:23:24 +0100, Patrick Wildt wrote:
> 
> > diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h 
> > b/gnu/usr.bin/binuti
> > ls-2.17/include/obstack.h
> > index 88c2a264adc..8839c48e95f 100644
> > --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> > +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > @@ -123,7 +123,7 @@ extern "C" {
> >  #endif
> >  
> >  #ifndef __INT_TO_PTR
> > -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> > +# define __INT_TO_PTR(P) ((char *)(P))
> >  #endif
> >  
> >  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is
> 
> Won't this cause a warning on 64-bit systems?  Maybe instead:
> 
> # define __INT_TO_PTR(P) ((char *)(intptr_t)(P))

Won't this sign extend the int?

> Also, what about __PTR_TO_INT?  Perhaps:
> 
> # define __PTR_TO_INT(P) ((int)(intptr_t)(P))

Won't the (int) cut off the value on 64-bit systems?

I'm not sure that's what they are after at this point.  I think what
they are doing is converting a pointer to a "number" to do arithmetics
on.  So maybe it's

# define __INT_TO_PTR(P) ((char *)(uintptr_t)(P))
# define __PTR_TO_INT(P) ((uintptr_t)(P))

Patrick



Re: binutils: build with LLVM 6.0.0

2018-03-31 Thread Patrick Wildt
On Thu, Mar 15, 2018 at 03:55:25PM -0700, William Ahern wrote:
> On Thu, Mar 15, 2018 at 05:23:24PM +0100, Patrick Wildt wrote:
> > Hi,
> > 
> > LLVM 6.0.0 does now complain of code does computation on NULL pointers,
> > which apparently binutils makes use of.  I think we can teach binutils
> > to stop doing that.
> > 
> > Is my C foo correct?  Feedback?
> 
> Both (type *)0 - 1 and (type *)-1 rely on undefined behavior, but
> _different_ undefined behavior. Even presuming the former was reliable, is
> the latter?
> 
> FWIW, the two expressions also evaluate to different values,
> 0xfffc vs 0x. That's not a problem by itself but
> suggests possible signedness (or whatever you call the analogous issue for
> pointer arithmetic) pitfalls.

You're right.  It does have a different value.  On the other hand, the
code is only used in three places.  One place checks for NULL, the other
two places are part of the diff.  And the comment above _bfd_elf_archive
_symbol_lookup actually says:

/* Return the linker hash table entry of a symbol that might be
   satisfied by an archive symbol.  Return -1 on error.  */

> Maybe it's safer to use the address of abfd, which should be shared even if
> archive_symbol_lookup is a function pointer to an external instantiation? I
> originally thought to use a static global singleton, but the macros and
> function pointers made me doubt the caller and callee are always from the
> same object. Using abfd is simpler and I don't see where abfd might be
> wrapped or proxied.

I don't understand what you're saying, I don't know the binutils code
well enough for that.

> > Patrick
> > 
> > diff --git a/gnu/usr.bin/binutils-2.17/bfd/elflink.c 
> > b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > index a6d17dcb4d9..aa010f9d0b2 100644
> > --- a/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > +++ b/gnu/usr.bin/binutils-2.17/bfd/elflink.c
> > @@ -4517,7 +4517,7 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
> >len = strlen (name);
> >copy = bfd_alloc (abfd, len);
> >if (copy == NULL)
> > -return (struct elf_link_hash_entry *) 0 - 1;
> > +return (struct elf_link_hash_entry *)-1;
> >  
> >first = p - name + 1;
> >memcpy (copy, name, first);
> > @@ -4629,7 +4629,7 @@ elf_link_add_archive_symbols (bfd *abfd, struct 
> > bfd_link_info *info)
> > }
> >  
> >   h = archive_symbol_lookup (abfd, info, symdef->name);
> > - if (h == (struct elf_link_hash_entry *) 0 - 1)
> > + if (h == (struct elf_link_hash_entry *)-1)
> > goto error_return;
> >  
> >   if (h == NULL)
> > diff --git a/gnu/usr.bin/binutils-2.17/include/obstack.h 
> > b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > index 88c2a264adc..8839c48e95f 100644
> > --- a/gnu/usr.bin/binutils-2.17/include/obstack.h
> > +++ b/gnu/usr.bin/binutils-2.17/include/obstack.h
> > @@ -123,7 +123,7 @@ extern "C" {
> >  #endif
> >  
> >  #ifndef __INT_TO_PTR
> > -# define __INT_TO_PTR(P) ((P) + (char *) 0)
> > +# define __INT_TO_PTR(P) ((char *)(P))
> >  #endif
> >  
> >  /* We need the type of the resulting object.  If __PTRDIFF_TYPE__ is



Re: dd(1): snprintf+writev -> dprintf

2018-03-31 Thread Theo de Raadt
> On Sat, Mar 31, 2018 at 01:04:43PM -0600, Theo de Raadt wrote:
> > The only worry about converting from iov to dprintf could be some
> > atomicity requirement.  I don't really see that in play here, I think
> > the iov uses was simply for convenience.  No other 'signalling interrupts'
> > seem to be in play.
> 
> My take was convenience, too, but in the commit you said it
> was for atomicity:
> 
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/bin/dd/misc.c?rev=1.9=text/x-cvsweb-markup
> 
> Is there anything else we're worried about here?
> 
> Rogue signals we can't anticipate leaving us with only partial
> output on stderr?

Let's see.  SIGINFO starts.  Imagine if the buffering is small (I
don't think it is).  If it was small, there could be multiple writes
to the sequence.  SIGINT arrives.  This will result in stderr output
being a bit garbled.  This isn't a huge problem in this circumstance,
but it demonstrates why atomicity may matter.  I think it can't occur
because the default buffer size is large enough?  ktrace can be used
to see what the dprintf is doing.




Re: dd(1): snprintf+writev -> dprintf

2018-03-31 Thread Scott Cheloha
On Sat, Mar 31, 2018 at 01:04:43PM -0600, Theo de Raadt wrote:
> The only worry about converting from iov to dprintf could be some
> atomicity requirement.  I don't really see that in play here, I think
> the iov uses was simply for convenience.  No other 'signalling interrupts'
> seem to be in play.

My take was convenience, too, but in the commit you said it
was for atomicity:

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/bin/dd/misc.c?rev=1.9=text/x-cvsweb-markup

Is there anything else we're worried about here?

Rogue signals we can't anticipate leaving us with only partial
output on stderr?

-Scott



arch/amd64 free

2018-03-31 Thread David Hill
Hello -

This diff is more involved.

In est_acpi_pss_changed, a new table is allocated but n isn't updated,
which keep tracks of the number allocated.  Set it.

In est_init, fake_table was being allocated with 3.  Change that to
allocate exactly what is need by setting n earlier.


Regarding the isa/ section, in _isa_bus_dmamap_create, cookiesize is:
  cookiesize = sizeof(struct isa_dma_cookie);
  ...
  if ((avail_end > ISA_DMA_BOUNCE_THRESHOLD &&
  (flags & ISABUS_DMA_32BIT) == 0) ||
  ((map->_dm_size / NBPG) + 1) > map->_dm_segcnt) {
cookieflags |= ID_MIGHT_NEED_BOUNCE;
cookiesize += (sizeof(bus_dma_segment_t) * map->_dm_segcnt);
  }

so I am assuming if ID_MIGHT_NEED_BOUNCE is set, that cookiesize gets
they larger size.

OK?

Index: arch/amd64/amd64/est.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/est.c,v
retrieving revision 1.39
diff -u -p -r1.39 est.c
--- arch/amd64/amd64/est.c  6 Mar 2016 22:41:24 -   1.39
+++ arch/amd64/amd64/est.c  31 Mar 2018 19:11:48 -
@@ -284,6 +284,7 @@ est_acpi_pss_changed(struct acpicpu_pss 
free(acpilist, M_DEVBUF, sizeof(struct fqlist));
return;
}
+   acpilist->n = npss;
 
for (i = 0; i < npss; i++) {
acpilist->table[i].mhz = pss[i].pss_core_freq;
@@ -377,10 +378,13 @@ est_init(struct cpu_info *ci)
"list\n", cpu_device);
return;
}
+   if (cur == idhi || cur == idlo)
+   fake_fqlist->n = 2; 
+   else
+   fake_fqlist->n = 3;
 
-
-   if ((fake_table = mallocarray(3, sizeof(struct est_op),
-   M_DEVBUF, M_NOWAIT)) == NULL) {
+   if ((fake_table = mallocarray(fake_fqlist->n,
+   sizeof(struct est_op), M_DEVBUF, M_NOWAIT)) == NULL) {
free(fake_fqlist, M_DEVBUF, sizeof(struct fqlist));
printf("%s: EST: cannot allocate memory for fake "
"table\n", cpu_device);
@@ -388,13 +392,12 @@ est_init(struct cpu_info *ci)
}
fake_table[0].ctrl = idhi;
fake_table[0].mhz = MSR2MHZ(idhi, bus_clock);
-   if (cur == idhi || cur == idlo) {
+   if (fake_fqlist->n == 2) {
printf("%s: using only highest and lowest power "
   "states\n", cpu_device);
 
fake_table[1].ctrl = idlo;
fake_table[1].mhz = MSR2MHZ(idlo, bus_clock);
-   fake_fqlist->n = 2;
} else {
printf("%s: using only highest, current and lowest "
"power states\n", cpu_device);
@@ -404,7 +407,6 @@ est_init(struct cpu_info *ci)
 
fake_table[2].ctrl = idlo;
fake_table[2].mhz = MSR2MHZ(idlo, bus_clock);
-   fake_fqlist->n = 3;
}
 
fake_fqlist->vendor = vendor;
@@ -441,7 +443,7 @@ est_init(struct cpu_info *ci)
return;
 
 nospeedstep:
-   free(est_fqlist->table, M_DEVBUF, 0);
+   free(est_fqlist->table, M_DEVBUF, est_fqlist->n * sizeof(struct 
est_op));
free(est_fqlist, M_DEVBUF, sizeof(*est_fqlist));
 }
 
Index: arch/amd64/isa/isa_machdep.c
===
RCS file: /cvs/src/sys/arch/amd64/isa/isa_machdep.c,v
retrieving revision 1.29
diff -u -p -r1.29 isa_machdep.c
--- arch/amd64/isa/isa_machdep.c14 Oct 2017 04:44:43 -  1.29
+++ arch/amd64/isa/isa_machdep.c31 Mar 2018 19:11:48 -
@@ -455,6 +455,7 @@ void
 _isa_bus_dmamap_destroy(bus_dma_tag_t t, bus_dmamap_t map)
 {
struct isa_dma_cookie *cookie = map->_dm_cookie;
+   size_t cookiesize = sizeof(struct isa_dma_cookie);
 
/*
 * Free any bounce pages this map might hold.
@@ -462,7 +463,10 @@ _isa_bus_dmamap_destroy(bus_dma_tag_t t,
if (cookie->id_flags & ID_HAS_BOUNCE)
_isa_dma_free_bouncebuf(t, map);
 
-   free(cookie, M_DEVBUF, 0);
+   if (cookie->id_flags & ID_MIGHT_NEED_BOUNCE)
+   cookiesize += (sizeof(bus_dma_segment_t) * map->_dm_segcnt);
+
+   free(cookie, M_DEVBUF, cookiesize);
_bus_dmamap_destroy(t, map);
 }
 



mfs free

2018-03-31 Thread David Hill
Add the free size. (allocated in mfs_vfsops.c)

mfsp = malloc(sizeof *mfsp, M_MFSNODE, M_WAITOK | M_ZERO);
devvp->v_data = mfsp;

OK?

Index: ufs/mfs/mfs_vnops.c
===
RCS file: /cvs/src/sys/ufs/mfs/mfs_vnops.c,v
retrieving revision 1.49
diff -u -p -r1.49 mfs_vnops.c
--- ufs/mfs/mfs_vnops.c 7 Nov 2016 00:26:33 -   1.49
+++ ufs/mfs/mfs_vnops.c 31 Mar 2018 19:11:58 -
@@ -238,7 +238,7 @@ mfs_reclaim(void *v)
struct vop_reclaim_args *ap = v;
struct vnode *vp = ap->a_vp;
 
-   free(vp->v_data, M_MFSNODE, 0);
+   free(vp->v_data, M_MFSNODE, sizeof(struct mfsnode));
vp->v_data = NULL;
return (0);
 }



netinet memcpy/free

2018-03-31 Thread David Hill
Hello -

memcpy can be used on freshly allocated memory.  Fill in the free size
for it.

OK?

Index: netinet/tcp_subr.c
===
RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.169
diff -u -p -r1.169 tcp_subr.c
--- netinet/tcp_subr.c  18 Mar 2018 21:05:21 -  1.169
+++ netinet/tcp_subr.c  31 Mar 2018 19:12:04 -
@@ -952,7 +952,7 @@ tcp_signature_tdb_init(struct tdb *tdbp,
tdbp->tdb_amxkey = malloc(ii->ii_authkeylen, M_XDATA, M_NOWAIT);
if (tdbp->tdb_amxkey == NULL)
return (ENOMEM);
-   bcopy(ii->ii_authkey, tdbp->tdb_amxkey, ii->ii_authkeylen);
+   memcpy(tdbp->tdb_amxkey, ii->ii_authkey, ii->ii_authkeylen);
tdbp->tdb_amxkeylen = ii->ii_authkeylen;
 
return (0);
@@ -963,7 +963,7 @@ tcp_signature_tdb_zeroize(struct tdb *td
 {
if (tdbp->tdb_amxkey) {
explicit_bzero(tdbp->tdb_amxkey, tdbp->tdb_amxkeylen);
-   free(tdbp->tdb_amxkey, M_XDATA, 0);
+   free(tdbp->tdb_amxkey, M_XDATA, tdbp->tdb_amxkeylen);
tdbp->tdb_amxkey = NULL;
}
 



Re: dd(1): snprintf+writev -> dprintf

2018-03-31 Thread Theo de Raadt
The only worry about converting from iov to dprintf could be some
atomicity requirement.  I don't really see that in play here, I think
the iov uses was simply for convenience.  No other 'signalling interrupts'
seem to be in play.



dd(1): snprintf+writev -> dprintf

2018-03-31 Thread Scott Cheloha
Simpler.

ok?

--
Scott Cheloha

Index: bin/dd/misc.c
===
RCS file: /cvs/src/bin/dd/misc.c,v
retrieving revision 1.22
diff -u -p -r1.22 misc.c
--- bin/dd/misc.c   24 Oct 2017 14:21:10 -  1.22
+++ bin/dd/misc.c   31 Mar 2018 17:49:42 -
@@ -36,13 +36,9 @@
 
 #include 
 #include 
-#include 
 
-#include 
-#include 
-#include 
-#include 
 #include 
+#include 
 #include 
 #include 
 
@@ -53,10 +49,7 @@ void
 summary(void)
 {
struct timespec elapsed, now;
-   char buf[4][100];
-   struct iovec iov[4];
double nanosecs;
-   int i = 0;
 
if (ddflags & C_NOINFO)
return;
@@ -67,38 +60,25 @@ summary(void)
if (nanosecs == 0)
nanosecs = 1;
 
-   /* Use snprintf(3) so that we don't reenter stdio(3). */
-   (void)snprintf(buf[0], sizeof(buf[0]),
-   "%zu+%zu records in\n%zu+%zu records out\n",
+   /* Be async safe: use dprintf(3). */
+   dprintf(STDERR_FILENO, "%zu+%zu records in\n%zu+%zu records out\n",
st.in_full, st.in_part, st.out_full, st.out_part);
-   iov[i].iov_base = buf[0];
-   iov[i++].iov_len = strlen(buf[0]);
 
if (st.swab) {
-   (void)snprintf(buf[1], sizeof(buf[1]),
-   "%zu odd length swab %s\n",
-st.swab, (st.swab == 1) ? "block" : "blocks");
-   iov[i].iov_base = buf[1];
-   iov[i++].iov_len = strlen(buf[1]);
+   dprintf(STDERR_FILENO, "%zu odd length swab %s\n",
+   st.swab, (st.swab == 1) ? "block" : "blocks");
}
if (st.trunc) {
-   (void)snprintf(buf[2], sizeof(buf[2]),
-   "%zu truncated %s\n",
-st.trunc, (st.trunc == 1) ? "block" : "blocks");
-   iov[i].iov_base = buf[2];
-   iov[i++].iov_len = strlen(buf[2]);
+   dprintf(STDERR_FILENO, "%zu truncated %s\n",
+   st.trunc, (st.trunc == 1) ? "block" : "blocks");
}
if (!(ddflags & C_NOXFER)) {
-   (void)snprintf(buf[3], sizeof(buf[3]),
+   dprintf(STDERR_FILENO,
"%lld bytes transferred in %lld.%03ld secs "
"(%0.0f bytes/sec)\n", (long long)st.bytes,
(long long)elapsed.tv_sec, elapsed.tv_nsec / 100,
((double)st.bytes * 10) / nanosecs);
-   iov[i].iov_base = buf[3];
-   iov[i++].iov_len = strlen(buf[3]);
}
-
-   (void)writev(STDERR_FILENO, iov, i);
 }
 
 void



Re: dd(1): overhaul operand parsing

2018-03-31 Thread Scott Cheloha
Updated diff with changes from tobias@:

This patch now removes (u_int) casts from input/output buffer
allocation in dd.c, which was incorrect beforehand anyway, but
with the other changes here was manifesting as a segfault for
combinations of cbs and ibs/obs that overflowed u_int.

--
Scott Cheloha

Index: bin/dd/args.c
===
RCS file: /cvs/src/bin/dd/args.c,v
retrieving revision 1.29
diff -u -p -r1.29 args.c
--- bin/dd/args.c   3 Jan 2018 19:12:20 -   1.29
+++ bin/dd/args.c   31 Mar 2018 17:47:04 -
@@ -39,8 +39,10 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -62,6 +64,9 @@ static void   f_skip(char *);
 static voidf_status(char *);
 static size_t  get_bsz(char *);
 static off_t   get_off(char *);
+static unsigned long long
+   get_expr(const char *, unsigned long long, unsigned long long);
+static long long get_multiplier(const char *);
 
 static const struct arg {
const char *name;
@@ -94,6 +99,7 @@ jcl(char **argv)
char *arg;
 
in.dbsz = out.dbsz = 512;
+   files_cnt = 1;
 
while ((oper = *++argv) != NULL) {
if ((oper = strdup(oper)) == NULL)
@@ -138,8 +144,6 @@ jcl(char **argv)
if (ddflags & (C_BLOCK|C_UNBLOCK)) {
if (!(ddflags & C_CBS))
errx(1, "record operations require cbs");
-   if (cbsz == 0)
-   errx(1, "cbs cannot be zero");
cfunc = ddflags & C_BLOCK ? block : unblock;
} else if (ddflags & C_CBS) {
if (ddflags & (C_ASCII|C_EBCDIC)) {
@@ -152,23 +156,14 @@ jcl(char **argv)
}
} else
errx(1, "cbs meaningless if not doing record 
operations");
-   if (cbsz == 0)
-   errx(1, "cbs cannot be zero");
} else
cfunc = def;
 
-   if (in.dbsz == 0 || out.dbsz == 0)
-   errx(1, "buffer sizes cannot be zero");
-
-   /*
-* Read and write take size_t's as arguments.  Lseek, however,
-* takes an off_t.
-*/
-   if (cbsz > SSIZE_MAX || in.dbsz > SSIZE_MAX || out.dbsz > SSIZE_MAX)
-   errx(1, "buffer sizes cannot be greater than %zd",
-   (ssize_t)SSIZE_MAX);
-   if (in.offset > LLONG_MAX / in.dbsz || out.offset > LLONG_MAX / 
out.dbsz)
-   errx(1, "seek offsets cannot be larger than %lld", LLONG_MAX);
+   /* Ensure the seek won't overflow. */
+   if (in.offset != 0 && in.dbsz > LLONG_MAX / in.offset)
+   errx(1, "total skip is too large");
+   if (out.offset != 0 && out.dbsz > LLONG_MAX / out.offset)
+   errx(1, "total seek is too large");
 }
 
 static int
@@ -196,15 +191,14 @@ static void
 f_count(char *arg)
 {
 
-   if ((cpy_cnt = get_bsz(arg)) == 0)
-   cpy_cnt = (size_t)-1;
+   cpy_cnt = get_expr(arg, 0, ULLONG_MAX);
 }
 
 static void
 f_files(char *arg)
 {
 
-   files_cnt = get_bsz(arg);
+   files_cnt = get_expr(arg, 0, ULLONG_MAX);
 }
 
 static void
@@ -307,165 +301,108 @@ f_conv(char *arg)
}
 }
 
-/*
- * Convert an expression of the following forms to a size_t
- * 1) A positive decimal number, optionally followed by
- * b - multiply by 512.
- * k, m or g - multiply by 1024 each.
- * w - multiply by sizeof int
- * 2) Two or more of the above, separated by x
- *(or * for backwards compatibility), specifying
- *the product of the indicated values.
- */
 static size_t
-get_bsz(char *val)
+get_bsz(char *arg)
 {
-   size_t num, t;
-   char *expr;
+   /* read(2) and write(2) can't handle more than SSIZE_MAX bytes. */
+   return get_expr(arg, 1, SSIZE_MAX);
+}
 
-   if (strchr(val, '-'))
-   errx(1, "%s: illegal numeric value", oper);
+static off_t
+get_off(char *arg)
+{
+   return get_expr(arg, 0, LLONG_MAX);
+}
 
-   errno = 0;
-   num = strtoul(val, , 0);
-   if (num == ULONG_MAX && errno == ERANGE)/* Overflow. */
-   err(1, "%s", oper);
-   if (expr == val)/* No digits. */
-   errx(1, "%s: illegal numeric value", oper);
+/*
+ * An expression is composed of one or more terms separated by exes ('x').
+ * Terms separated by asterisks ('*') are supported for backwards
+ * compatibility.  The value of an expression is the product of its terms.
+ *
+ * A term is composed of a non-negative decimal integer and an optional
+ * suffix of a single character.  Suffixes are valued as follows:
+ *
+ * b   - 512   ("block")
+ * G|g - 1073741824("gigabyte")
+ * K|k - 1024  ("kilobyte")
+ * M|m - 1048576   ("megabyte")
+ * w   - sizeof(int)   ("word")
+ *
+ * The value of a term without a suffix is that 

Re: Should rm(1) -Pf change file permission?

2018-03-31 Thread Ingo Schwarze
Hi,

Gregoire Jadi wrote on Fri, Mar 30, 2018 at 06:07:42PM +0200:

> While working on a port of keyringer, I observed the following behavior
> of rm(1) with the -P option: if the file does not have write permission,
> the file is removed without being overwritten.
> 
> This is not the same behavior as shred(1) (from sysutils/coreutils) which
> do not remove the file if it cannot be overwritten. If the -f option is
> used with shred(1), the permission of the file are changed to allow
> writing if necessary.
> 
> Is the current behavior desired? (need to update the manpage?)
> 
> Or should `rm -P` and `rm -Pf` have the same behavior as shred. That is:
> `rm -P` should fail without removing the file if the file cannot be
> overwritten and `rm -Pf` should change the permission and overwrite
> file.

That makes more sense to me than the current behaviour.

In addition, when running with -P and without -f and the user
interactive confirms that the missing write protection shall be
overriden, changing the permissions seems right to me, too.

Here is a patch implementing that.

Lightly tested by hand by running various combinations of rm(1)
 * with and without -P
 * with and without -f
 * on files with different permissions

Feedback is welcome both on the general idea and on the specific
implementation.

Note that the return value of rm_overwrite() is currently unused.
So we are free to change it to reflect success or failure, such
that deleting the file can be skipped in case of failure even
after trying to relax the permissions.  Also note that checking
fflag in rw_overwrite is not needed (and would even be wrong)
because the program doesn't get there in the first place unless
fflag was given or deletion was interactively confirmed.

Yours,
  Ingo


Index: rm.c
===
RCS file: /cvs/src/bin/rm/rm.c,v
retrieving revision 1.42
diff -u -p -r1.42 rm.c
--- rm.c27 Jun 2017 21:49:47 -  1.42
+++ rm.c31 Mar 2018 17:02:37 -
@@ -103,7 +103,7 @@ main(int argc, char *argv[])
argv += optind;
 
if (Pflag) {
-   if (pledge("stdio rpath wpath cpath getpw", NULL) == -1)
+   if (pledge("stdio rpath wpath cpath fattr getpw", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath cpath getpw", NULL) == -1)
@@ -213,9 +213,9 @@ rm_tree(char **argv)
 
case FTS_F:
case FTS_NSOK:
-   if (Pflag)
-   rm_overwrite(p->fts_accpath, p->fts_info ==
-   FTS_NSOK ? NULL : p->fts_statp);
+   if (Pflag && !rm_overwrite(p->fts_accpath,
+   p->fts_info == FTS_NSOK ? NULL : p->fts_statp))
+   continue;
/* FALLTHROUGH */
default:
if (!unlink(p->fts_accpath) ||
@@ -264,8 +264,8 @@ rm_file(char **argv)
else if (S_ISDIR(sb.st_mode))
rval = rmdir(f);
else {
-   if (Pflag)
-   rm_overwrite(f, );
+   if (Pflag && !rm_overwrite(f, ))
+   continue;
rval = unlink(f);
}
if (rval && (!fflag || errno != ENOENT)) {
@@ -308,9 +308,12 @@ rm_overwrite(char *file, struct stat *sb
if (sbp->st_nlink > 1) {
warnx("%s (inode %llu): not overwritten due to multiple links",
file, (unsigned long long)sbp->st_ino);
-   return (0);
+   return (1);
}
-   if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1)
+   if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1 &&
+   (errno != EACCES ||
+chmod(file, sb.st_mode | S_IWUSR) == -1 ||
+(fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1))
goto err;
if (fstat(fd, ))
goto err;



[PATCH] Update default QoS markers for ssh

2018-03-31 Thread Job Snijders
Dear all,

There may be opportunity for improvement of ssh(1) and sshd(8)'s default
QoS markers for better integration in environments that can offer either
layer-2 or layer-3 prioritisation profiles. Currently ssh(1) and sshd(8)
set obsoleted values 'lowdelay' for interactive sessions and
'throughput' for non-interactive sessions.

TL;DR: I propose to update the defaults to use DSCP "AF21" (Low Latency
Data) for interactive session traffic, and CS1 ("Lower Effort") for
non-interactive traffic. This applies to both IPv4 and IPv6.

The openbsd 'lowdelay' value translates to IP TOS '0x10', and
'throughput' translates to IP TOS 0x08'. The IP TOS field is 8 bits, and
was defined in RFC 1349 (1992 A.D.). IP TOS was deprecated in RFC 2474
(1998 A.D.) with the introduction of "Differentiated Services Field" aka
DSCP.

When the world transitioned from TOS to DSCP, the IP TOS field was
chopped up into two parts: the 6 most significant bits became the
DSField (which contains a DSCP), and the 2 least significant bits became
a place for ECN experimentation. (Similarly in IPv6, the 'Traffic Class'
became the DSField). To convert the IP TOS value to a DSCP value you
shift the value 2 bits to the right: 'lowdelay' = DSCP 0x04 and
'throughput' = DSCP 0x02. It should be noted that DSCP 0x04 and DSCP
0x02 have no meaning. Both 0x02 and 0x04 map to Precedence "routine",
which is the /least/ important Precedence.

Using IP TOS values is problematic because network operators can't match
on TOS values on today's equipment. We looked at Cisco IOS XR, Juniper
Junos, and Nokia SR-OS - and there aren't classifiers to match on TOS
values. Instead, the systems I mentioned allow you to match on DSCP
value (and perhaps IPv4 precedence).

The ability to correctly match QoS markers of this nature is important
in controlled environments, you'll generally see that the DSCP values
are fully trusted, or not honored at all (such as when crossing Internet
boundaries). An example would be how traffic from VOIP phones/apps are
treated in enterprise office networks, perhaps even in the entire branch
network. The industry practise is that iff QoS markers are honored, they
are translated 1:1 - so AF21 goes into AF21 (or equivalent).

I selected AF21 as this is the highest priority within the low-latency
service class (and it is higher than what we have today). SSH is elastic
and time-sensitive data, where a user is waiting for a response via the
network in order to continue with a task at hand. As such, these flows
should be considered foreground traffic, with delays or drops to such
traffic directly impacting user-productivity.

You may ask, "why not EF?" - the risk with EF is that you can easily end
up in a tiny policer queue, as an example some CPEs by default only
allow up to 100Kbps of EF traffic. EF is meant for inelastic traffic,
where for instance the codec sends packets at the rate the codec
produces them, regardless of availability of capacity.

For bulk SSH traffic, the "Lower Effort" marker was chosen to enable
networks implementing a scavanger/lower-than-best effort class to
discriminate scp(1) below normal activities, such as web surfing. In
general this type of bulk SSH traffic is a background activity. Today
this traffic would go into 'best effort', potentially drowning out other
traffic on wireless links, so kicking this into 'background' may improve
the user experience.

An advantage of using "AF21" for interactive SSH and "CS1" for bulk SSH
is that they are recognisable values on all common platforms (IANA
https://www.iana.org/assignments/dscp-registry/dscp-registry.xml ), and
for AF21 specifically a rigorous definition of the intended behavior
exists https://tools.ietf.org/html/rfc4594#section-4.7 in addition to
the definition of the Assured Forwarding PHB group
https://tools.ietf.org/html/rfc2597, and for CS1 (Lower Effort) there is
https://tools.ietf.org/html/rfc3662 - finally this document is also a
recommened reading: https://tools.ietf.org/html/rfc8325

Another advantage is that the first three bits of "AF21" map to the
equivalent I 802.1D PCP, IEEE 802.11e, MPLS EXP/Cos and IP
Precedence value of 2 (also known as "Immediate", or "AC_BE"), and CS1's
first 3 bits map to I 802.1D PCP, IEEE 802.11e, MPLS/Cos and IP
Precedence value 1 ("Background" or "AC_BK"). 

The below patch ensures that in environments where people are using QoS,
the openssh defaults can easily matched both for interactive and bulk
SSH traffic in all layers of the transport stack (wired/wirless
Ethernet, MPLS, IP) and priortize accordingly.

Kind regards,

Job

The below patch should be portable, but I've only tested it on my own
openbsd machines.

 usr.bin/ssh/readconf.c| 4 ++--
 usr.bin/ssh/servconf.c| 4 ++--
 usr.bin/ssh/ssh_config.5  | 6 --
 usr.bin/ssh/sshd_config.5 | 6 --
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git usr.bin/ssh/readconf.c usr.bin/ssh/readconf.c
index 5d17b725600..d3a121373d8 100644
--- 

Re: libsa: udp read changes endianness

2018-03-31 Thread Theo de Raadt
> I think we should not convert headers into a host-readable format, the
> upper layers need to know the network layers if they do this kind of
> layer overreach.

I agree.  It probably comes from a time when ntohs and family were true
functions (during the inline asm conversion days); it was seen as an
optimization but nowadays we know this is premature conversion placement.



Re: manpage text width

2018-03-31 Thread Theo de Raadt
> Don't you already need this logic, getenv(3) + isatty(3), to decide if
> you use a pager or not?  Although I don't understand why getenv(3) is
> needed, isn't a TIOCGWINSZ ioctl(2) enough?.

Because there is a defacto standard that some SVR2 environment (before
the ioctl) be honoured by many programs.  Few comments: Scripts can
tweak them to create output for a specific layout.  They allow
adjustment on non-tty circumstance.  The priority of getenv vs tty
layout vs default isn't precisely specified anywhere, and is a mess I
wish someone would study and try to cleanup.  During pledge
development, this stuff stuff in the way, it was quite interesting - I
spent the entire Exeter hackathon on it.



Re: expr: Fix integer overflows

2018-03-31 Thread Ingo Schwarze
Hi Tobias,

Tobias Stoeckmann wrote on Sat, Mar 31, 2018 at 01:28:19PM +0200:

> I actually ended up in expr(1) after seeing that the test(1) and ksh(1)
> debate could be continued here. While expr(1) is int64_t, expressions
> in ksh(1) are of type long, i.e. 32/64 bit depending on architecture.
> 
> This patch therefore is a preparation to get test(1) and expr(1) changes
> into ksh. As both utilities are standalone, it is much easier to discuss
> and make modifications in them, first.

All that sounds reasonable to me.

> Your example also triggers an overflow in FreeBSD's expr, so it is
> a good idea to inform them when we are happy with our fixed version.

Yes, please do that.

> Adjusted patch

Three more nits inline, and the updated patch appended at the end.

With that, OK schwarze@.
  Ingo


> Index: bin/expr/expr.c
> ===
> RCS file: /cvs/src/bin/expr/expr.c,v
> retrieving revision 1.26
> diff -u -p -u -p -r1.26 expr.c
> --- bin/expr/expr.c   19 Oct 2016 18:20:25 -  1.26
> +++ bin/expr/expr.c   31 Mar 2018 11:22:05 -
> @@ -19,8 +19,8 @@
>  struct val   *make_int(int64_t);
>  struct val   *make_str(char *);
>  void  free_value(struct val *);
> -int   is_integer(struct val *, int64_t *);
> -int   to_integer(struct val *);
> +int   is_integer(struct val *, int64_t *, const char **);
> +int   to_integer(struct val *, const char **);
>  void  to_string(struct val *);
>  int   is_zero_or_null(struct val *);
>  void  nexttoken(int);
> @@ -94,11 +94,9 @@ free_value(struct val *vp)
>  
>  /* determine if vp is an integer; if so, return it's value in *r */
>  int
> -is_integer(struct val *vp, int64_t *r)
> +is_integer(struct val *vp, int64_t *r, const char **errstr)
>  {
> - char   *s;
> - int neg;
> - int64_t i;
> + const char *errstrp;

Even if the current code doesn't rely on it, i think we should adopt
the good practice of strtonum(3) here of setting *errstr to NULL
on success, to make future bugs less likely.

Consider inserting at this point:

if (errstr == NULL)
errstr = 
*errstr = NULL;

>   if (vp->type == integer) {
>   *r = vp->u.i;
> @@ -107,43 +105,29 @@ is_integer(struct val *vp, int64_t *r)
>  
>   /*
>* POSIX.2 defines an "integer" as an optional unary minus
> -  * followed by digits.
> +  * followed by digits. Other representations are unspecified,
> +  * which means that strtonum(3) is a viable option here.
>*/
> - s = vp->u.s;
> - i = 0;
> + if (errstr == NULL)
> + errstr = 

These two lines can then be removed.

> + *r = strtonum(vp->u.s, INT64_MIN, INT64_MAX, errstr);
> + if (*errstr != NULL)
> + return 0;

This can be simplified to

return *errstr == NULL;

> - neg = (*s == '-');
> - if (neg)
> - s++;
> -
> - while (*s) {
> - if (!isdigit((unsigned char)*s))
> - return 0;
> -
> - i *= 10;
> - i += *s - '0';
> -
> - s++;
> - }
> -
> - if (neg)
> - i *= -1;
> -
> - *r = i;
>   return 1;
>  }
>  
>  
>  /* coerce to vp to an integer */
>  int
> -to_integer(struct val *vp)
> +to_integer(struct val *vp, const char **errstr)
>  {
>   int64_t r;
>  
>   if (vp->type == integer)
>   return 1;

For the reason explained above, insert here:

if (errstr != NULL)
*errstr = NULL;

> - if (is_integer(vp, )) {
> + if (is_integer(vp, , errstr)) {
>   free(vp->u.s);
>   vp->u.i = r;
>   vp->type = integer;
> @@ -176,7 +160,7 @@ is_zero_or_null(struct val *vp)
>   if (vp->type == integer)
>   return vp->u.i == 0;
>   else
> - return *vp->u.s == 0 || (to_integer(vp) && vp->u.i == 0);
> + return *vp->u.s == 0 || (to_integer(vp, NULL) && vp->u.i == 0);
>  }
>  
>  void
> @@ -303,20 +287,25 @@ eval5(void)
>  struct val *
>  eval4(void)
>  {
> - struct val *l, *r;
> - enum token  op;
> + const char  *errstr;
> + struct val  *l, *r;
> + enum token   op;
> + volatile int64_t res;
>  
>   l = eval5();
>   while ((op = token) == MUL || op == DIV || op == MOD) {
>   nexttoken(0);
>   r = eval5();
>  
> - if (!to_integer(l) || !to_integer(r)) {
> - errx(2, "non-numeric argument");
> + if (!to_integer(l, ) || !to_integer(r, )) {
> + errx(2, "%s", errstr);

I suggest to be more helpful here:

if (!to_integer(l, ))
errx(2, "number \"%s\" is %s", l->u.s, errstr);
if (!to_integer(r, ))
errx(2, "number \"%s\" is %s", r->u.s, errstr);

>   }
>  
>   

Re: manpage text width

2018-03-31 Thread Lars Noodén
On 3/31/18, Andras Farkas wrote:
> On Fri, Mar 30, 2018 at 11:23 AM, Chris Bennett wrote:
>> This is very important. Our brains just are not good at working with
>> long lines. This is hard-wired. If anyone doesn't believe me, try
>> setting your browser window to a narrower width or use reader mode.
>> We read by mapping things out on the line. If it's too long, our brains
>> get "confused" and information is lost.
> Is there any research backing this up?
[snip]

Yes and no.  The preferences of long or short lines might depend on
the individual doing the reading.  There is a study which seems highly
visible despite the small data set:

"No effects of line length were found for comprehension or
satisfaction, however, users indicated a strong preference
for either the short or long line lengths."

>From "The Effects of Line Length on Reading Online News" (2005)
(Warning for PDF)
http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.503.2885=rep1=pdf

But there is a generally accepted optimal range allegedly listed in
Robert  Bringhurst's "The Elements of Typographic Style" :

"Anything from 45 to 75 characters is widely-regarded as a
satisfactory length of line for a single-column page set in
a serifed text face in a text size. The 66-character line
(counting both letters and spaces) is widely regarded as
ideal."

quoted from "The Line Length Misconception"
https://www.viget.com/articles/the-line-length-misconception/

/Lars



Re: manpage text width

2018-03-31 Thread Marc Espie
On Fri, Mar 30, 2018 at 01:57:43AM +0200, Ingo Schwarze wrote:
> I do *NOT* want to add SIGWINCH signal handling to man(1) to abort
> less(1), reformat, and respawn less(1) in that case.  That kind of
> magic would be over the top, and SIGWINCH is an abomination in the
> first place.

Why on earth would you think so ?...

I've been dealing with SIGWINCH without real issues in a few programs
and it works really nice.



Re: manpage text width

2018-03-31 Thread Andras Farkas
On Fri, Mar 30, 2018 at 11:23 AM, Chris Bennett
 wrote:
> This is very important. Our brains just are not good at working with
> long lines. This is hard-wired. If anyone doesn't believe me, try
> setting your browser window to a narrower width or use reader mode.
> We read by mapping things out on the line. If it's too long, our brains
> get "confused" and information is lost.
Is there any research backing this up?  I've seen this sentiment twice
in this thread, but man's column limit has always been the most
dreaded part about man for me. (I'm very happy to find I can change it
with -Owidth)  I've found that doubling it to 160 columns has always
been far, far more comfortable for me, so I have doubt that the 80/78
column limit is anything more than a tradition looking for any reason
to continue existing.
If a user resizes their terminal before running a program, it makes
perfect sense for text to wrap to the terminal's size, just like with
programs that simply use stdout to output text.



Re: expr: Fix integer overflows

2018-03-31 Thread Tobias Stoeckmann
Hi,

On Sat, Mar 31, 2018 at 02:57:45AM +0200, Ingo Schwarze wrote:
> Even though - as discussed previously for test(1) - behaviour is
> undefined by POSIX outside the range 0 to 2147483647, we are allowed
> to handle a larger range, and i agree that handling the range
> -9223372036854775808 to 9223372036854775807 (INT64_MIN to INT64_MAX)
> seems desirable.

I actually ended up in expr(1) after seeing that the test(1) and ksh(1)
debate could be continued here. While expr(1) is int64_t, expressions
in ksh(1) are of type long, i.e. 32/64 bit depending on architecture.

This patch therefore is a preparation to get test(1) and expr(1) changes
into ksh. As both utilities are standalone, it is much easier to discuss
and make modifications in them, first.

> > +   if (c < 0 || INT64_MAX / 10 < i || INT64_MAX - c < i * 10)
> > +   errx(3, "out of range");
> 
> I'm not saying this is an absolute show-stopper, but i consider
> it somewhat ugly.  What do you think about using strtonum(3)
> instead in is_integer?

Totally agree. Patch is adjusted. I tried to keep it as strict as it
was before, but you are correct. We don't violate POSIX with strtonum,
we just accept a few more inputs -- being in sync with ksh and test.

>$ /obin/expr.tobias -36854775808 - \( -9223372036854775807 - 1 \)
>   expr.tobias: overflow
> 
>   if ((l->u.i >= 0 && r->u.i < 0 && res <= 0) ||

I have adjusted the code. Awesome review skills and thanks a lot! Your
example also triggers an overflow in FreeBSD's expr, so it is a good
idea to inform them when we are happy with our fixed version.

> Do you want to check and re-send your patch?

Most certainly. Adjusted patch and extended regress tests in this mail.
Thanks a lot again!


Tobias

Index: bin/expr/expr.c
===
RCS file: /cvs/src/bin/expr/expr.c,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 expr.c
--- bin/expr/expr.c 19 Oct 2016 18:20:25 -  1.26
+++ bin/expr/expr.c 31 Mar 2018 11:22:05 -
@@ -19,8 +19,8 @@
 struct val *make_int(int64_t);
 struct val *make_str(char *);
 voidfree_value(struct val *);
-int is_integer(struct val *, int64_t *);
-int to_integer(struct val *);
+int is_integer(struct val *, int64_t *, const char **);
+int to_integer(struct val *, const char **);
 voidto_string(struct val *);
 int is_zero_or_null(struct val *);
 voidnexttoken(int);
@@ -94,11 +94,9 @@ free_value(struct val *vp)
 
 /* determine if vp is an integer; if so, return it's value in *r */
 int
-is_integer(struct val *vp, int64_t *r)
+is_integer(struct val *vp, int64_t *r, const char **errstr)
 {
-   char   *s;
-   int neg;
-   int64_t i;
+   const char *errstrp;
 
if (vp->type == integer) {
*r = vp->u.i;
@@ -107,43 +105,29 @@ is_integer(struct val *vp, int64_t *r)
 
/*
 * POSIX.2 defines an "integer" as an optional unary minus
-* followed by digits.
+* followed by digits. Other representations are unspecified,
+* which means that strtonum(3) is a viable option here.
 */
-   s = vp->u.s;
-   i = 0;
+   if (errstr == NULL)
+   errstr = 
+   *r = strtonum(vp->u.s, INT64_MIN, INT64_MAX, errstr);
+   if (*errstr != NULL)
+   return 0;
 
-   neg = (*s == '-');
-   if (neg)
-   s++;
-
-   while (*s) {
-   if (!isdigit((unsigned char)*s))
-   return 0;
-
-   i *= 10;
-   i += *s - '0';
-
-   s++;
-   }
-
-   if (neg)
-   i *= -1;
-
-   *r = i;
return 1;
 }
 
 
 /* coerce to vp to an integer */
 int
-to_integer(struct val *vp)
+to_integer(struct val *vp, const char **errstr)
 {
int64_t r;
 
if (vp->type == integer)
return 1;
 
-   if (is_integer(vp, )) {
+   if (is_integer(vp, , errstr)) {
free(vp->u.s);
vp->u.i = r;
vp->type = integer;
@@ -176,7 +160,7 @@ is_zero_or_null(struct val *vp)
if (vp->type == integer)
return vp->u.i == 0;
else
-   return *vp->u.s == 0 || (to_integer(vp) && vp->u.i == 0);
+   return *vp->u.s == 0 || (to_integer(vp, NULL) && vp->u.i == 0);
 }
 
 void
@@ -303,20 +287,25 @@ eval5(void)
 struct val *
 eval4(void)
 {
-   struct val *l, *r;
-   enum token  op;
+   const char  *errstr;
+   struct val  *l, *r;
+   enum token   op;
+   volatile int64_t res;
 
l = eval5();
while ((op = token) == MUL || op == DIV || op == MOD) {
nexttoken(0);
r = eval5();
 
-   if (!to_integer(l) || !to_integer(r)) {
-   errx(2, "non-numeric 

Re: manpage text width

2018-03-31 Thread Martin Pieuchot
On 31/03/18(Sat) 03:20, Ingo Schwarze wrote:
> Paul Irofti wrote on Fri, Mar 30, 2018 at 11:23:54AM +0300:
> [...] 
> > which is EXACTLY what I was looking for! Can it be the default? :)
> 
> I'm neither particularly enthusiastiastic (because it requires
> more code, in particular more getenv(3) which i dislike in general,
> and because it adds another difference in behaviour depending on
> isatty(3)) nor am im categorically rejecting the request.

Don't you already need this logic, getenv(3) + isatty(3), to decide if
you use a pager or not?  Although I don't understand why getenv(3) is
needed, isn't a TIOCGWINSZ ioctl(2) enough?.

> If a few developers consider the feature useful, i shall implement
> it; if nobody else speaks up, i'm more likely to KISS it.

I'd greatly appreciate this default as well.



Re: Should rm(1) -Pf change file permission?

2018-03-31 Thread Craig Skinner
Hi Grégoire/all,

On Fri, 30 Mar 2018 18:07:42 +0200 Grégoire Jadi wrote:
> ... here is a small test to demonstrate ...

Same behaviour noticed and previously bugged:-
http://openbsd-archive.7691.n7.nabble.com/rm-P-doesn-t-overwrite-a-user-owned-read-only-file-td266276.html

Regards,
-- 
Craig Skinner | http://linkd.in/yGqkv7



libsa: udp read changes endianness

2018-03-31 Thread Patrick Wildt
Hi,

I have been working on TFTP boot support for arm64 and armv7 on top of
u-boot.  One thing that set me back was an endianness issue.  TFTP works
the way that you send to port 69, but when the server answers he chooses
a new source port.  So when you reply again, you have to reply to the
new source port.

At this point my TFTP failed, because as it turns out sendudp() expects
a packet where everything is in network-endianness, but readudp() does
"help" the user and converts the headers to host-endianness.  So when
we read the source port and throw it back into the dest port, it's of
the wrong endianness.

Why has this never been observed before?

 * Only amd64 and i386 include tftp.c, but they use an MD sendudp() and
   readudp() that works completely different.
 * Only macppc and sparc64 use netudp.c, which does the conversion, but
   I think those are big-endian anyway.  They have no tftp.c.  Also I
   don't think macppc actually has any netudp.c users.
 * Even if they were little-endian only bootp.c and rpc.c use readudp().
   tftp.c, which uses it, is not compiled-in.
 * From those only rpc.c uses the UDP header.  While rpc_fromaddr() does
   return the source port, the caller immediately replaces it with a
   pre-defined (network endian) value.  If that wasn't the case, the
   port would definitely be wrong.

Point being: No architecture has that issue, and no architecture does
care if I change the endianness.  But it makes arm64 work.

Oh, and as it turns out: ip->ip_len is also converted, but no one uses
it.

I think we should not convert headers into a host-readable format, the
upper layers need to know the network layers if they do this kind of
layer overreach.

Opinions? ok?

Patrick

diff --git a/sys/lib/libsa/netudp.c b/sys/lib/libsa/netudp.c
index 0199caaede5..1c913d4099a 100644
--- a/sys/lib/libsa/netudp.c
+++ b/sys/lib/libsa/netudp.c
@@ -183,11 +183,11 @@ readudp(struct iodesc *d, void *pkt, size_t len, time_t 
tleft)
 #endif
return -1;
}
-   ip->ip_len = ntohs(ip->ip_len);
-   if (n < ip->ip_len) {
+   if (n < ntohs(ip->ip_len)) {
 #ifdef NET_DEBUG
if (debug)
-   printf("readudp: bad length %d < %d.\n", n, ip->ip_len);
+   printf("readudp: bad length %d < %d.\n",
+   n, ntohs(ip->ip_len));
 #endif
return -1;
}
@@ -204,7 +204,7 @@ readudp(struct iodesc *d, void *pkt, size_t len, time_t 
tleft)
/* If there were ip options, make them go away */
if (hlen != sizeof(*ip)) {
bcopy(((u_char *)ip) + hlen, uh, len - hlen);
-   ip->ip_len = sizeof(*ip);
+   ip->ip_len = htons(sizeof(*ip));
n -= hlen - sizeof(*ip);
}
if (uh->uh_dport != d->myport) {
@@ -238,14 +238,11 @@ readudp(struct iodesc *d, void *pkt, size_t len, time_t 
tleft)
}
*ip = tip;
}
-   uh->uh_dport = ntohs(uh->uh_dport);
-   uh->uh_sport = ntohs(uh->uh_sport);
-   uh->uh_ulen = ntohs(uh->uh_ulen);
-   if (uh->uh_ulen < sizeof(*uh)) {
+   if (ntohs(uh->uh_ulen) < sizeof(*uh)) {
 #ifdef NET_DEBUG
if (debug)
printf("readudp: bad udp len %d < %d\n",
-   uh->uh_ulen, sizeof(*uh));
+   ntohs(uh->uh_ulen), sizeof(*uh));
 #endif
return -1;
}