Re: /bin/cp: Incorrect checking the return value
From: "Theo de Raadt"
Date: Fri, 16 Aug 2019 10:01:25 -0600
> Masato Asou wrote:
>
>> Incorrect checking the return value of malloc and system calls in
>> /bin/cp.
>
> The NULL vs ! checks, I cannot agree with those. Their effect is
> identical and it is commonplace to use either idiom.
I accept your oppinion.
> As to precise-checks for system call return values of -1 or 0, vs
> <, for that case I do not know how to proceed.
>
> I actually have a diff lying around which does precise -1 checks for the
> entire tree, in regards to section-2 system calls and some of the
> thinner section-3 wrappers.
>
> I wanted to know if there were any truly incorrect checks, and the
> easiest way was to carefully adjust all the code, and then read the
> diffs afterwards. I found only two bugs, which I fixed a couple months
> ago. So this is the remainder.
>
> In the following diff, I believe the <, <= and != 0 being converted
> to precise == 0 and == -1 are more precise but without error, but I'm
> not sure whether to proceed. On some architectures, binaries will
> grow minutely larger because a precise check uses a few more instruction
> bytes than a relative range check.
>
> BTW, there IS some opportunity for making mistakes with such a diff,
> especially around the 64-bit system calls and thin wrappers.
>
> So I don't think you should proceed with precise system call checks for
> this one program, until we all discuss whether this idiom is valuable to
> do for ALL programs. Is precise -1 checking a more correct idiom, or is
> it pointless?
I agree that we need to discuss about this idiom
Thank your.
--
ASOU Masato
> (As I said, I'm ok with deleting my 1750 line diff since (1) I have
> started thinking it is pointless, and (2) it was purely the side effort
> of an audit procedure)
>
> Index: usr.bin/biff/biff.c
> ===
> RCS file: /cvs/src/usr.bin/biff/biff.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 biff.c
> --- usr.bin/biff/biff.c 28 Jun 2019 13:35:00 - 1.17
> +++ usr.bin/biff/biff.c 4 Jul 2019 17:00:26 -
> @@ -66,7 +66,7 @@ main(int argc, char *argv[])
> if (pledge("stdio rpath fattr", NULL) == -1)
> err(2, "pledge");
>
> - if (stat(name, ))
> + if (stat(name, ) == -1)
> err(2, "stat");
>
> sb.st_mode &= ACCESSPERMS;
> Index: usr.bin/calendar/calendar.c
> ===
> RCS file: /cvs/src/usr.bin/calendar/calendar.c,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 calendar.c
> --- usr.bin/calendar/calendar.c 1 Feb 2019 16:22:53 - 1.37
> +++ usr.bin/calendar/calendar.c 6 Jul 2019 15:40:49 -
> @@ -167,21 +167,21 @@ main(int argc, char *argv[])
>* we can chdir() we can stat(), unless the user is
>* modifying permissions while this is running.
>*/
> - if (chdir(pw->pw_dir)) {
> + if (chdir(pw->pw_dir) == -1) {
> if (errno == EACCES)
> acstat = 1;
> else
> continue;
> }
> - if (stat(calendarFile, ) != 0) {
> - if (chdir(calendarHome)) {
> + if (stat(calendarFile, ) == -1) {
> + if (chdir(calendarHome) == -1) {
> if (errno == EACCES)
> acstat = 1;
> else
> continue;
> }
> if (stat(calendarNoMail, ) == 0 ||
> - stat(calendarFile, ) != 0)
> + stat(calendarFile, ) == -1)
> continue;
> }
> sleeptime = USERTIMEOUT;
> @@ -197,11 +197,11 @@ main(int argc, char *argv[])
> err(1, "unable to set user context (uid
> %u)",
> pw->pw_uid);
> if (acstat) {
> - if (chdir(pw->pw_dir) ||
> - stat(calendarFile, ) != 0 ||
> - chdir(calendarHome) ||
> + if (chdir(pw->pw_dir) == -1 ||
> + stat(calendarFile, ) == -1 ||
> + chdir(calendarHome) == -1 ||
> stat(calendarNoMail, ) == 0 ||
> - stat(calendarFile, ) != 0)
> +
wsmouse(4): reverse scroll directions
With the introduction of precision scrolling, it is no longer possible to
reverse scroll directions for touchpads by means of button mappings in X.
This patch adds a configuration option to wsmouse and a boolean field
named
wsmouse*.reverse_scrolling
to wsconsctl. The option is valid for both mice and touchpads; if it has
a non-zero value, wsmouse inverts the sign of the values of wheel events
and of the scroll events generated by touchpads.
Apple has made the inverted mapping popular as "natural scrolling", and
some people may even attribute the opposite meaning to the field name.
However, I couldn't persuade myself to declare that the classical way of
scrolling is less "natural".
OK?
Index: sys/dev/wscons/wsconsio.h
===
RCS file: /cvs/src/sys/dev/wscons/wsconsio.h,v
retrieving revision 1.91
diff -u -p -r1.91 wsconsio.h
--- sys/dev/wscons/wsconsio.h 24 Mar 2019 17:55:39 - 1.91
+++ sys/dev/wscons/wsconsio.h 18 Aug 2019 17:47:39 -
@@ -292,6 +292,8 @@ enum wsmousecfg {
WSMOUSECFG_SWAPXY, /* swap X- and Y-axis */
WSMOUSECFG_X_INV, /* map absolute coordinate X to (INV - X) */
WSMOUSECFG_Y_INV, /* map absolute coordinate Y to (INV - Y) */
+ WSMOUSECFG_REVERSE_SCROLLING,
+ /* reverse scroll directions */
/*
* Coordinate handling, applying only in WSMOUSE_COMPAT mode.
@@ -343,7 +345,7 @@ enum wsmousecfg {
WSMOUSECFG_LOG_INPUT = 256,
WSMOUSECFG_LOG_EVENTS,
};
-#define WSMOUSECFG_MAX 38 /* max size of param array per ioctl */
+#define WSMOUSECFG_MAX 39 /* max size of param array per ioctl */
struct wsmouse_param {
enum wsmousecfg key;
Index: sys/dev/wscons/wsmouse.c
===
RCS file: /cvs/src/sys/dev/wscons/wsmouse.c,v
retrieving revision 1.55
diff -u -p -r1.55 wsmouse.c
--- sys/dev/wscons/wsmouse.c24 May 2019 06:05:38 - 1.55
+++ sys/dev/wscons/wsmouse.c18 Aug 2019 17:47:39 -
@@ -1018,7 +1018,7 @@ wsmouse_motion_sync(struct wsmouseinput
struct motion_state *motion = >motion;
struct axis_filter *h = >filter.h;
struct axis_filter *v = >filter.v;
- int x, y, dx, dy;
+ int x, y, dx, dy, dz, dw;
if (motion->sync & SYNC_DELTAS) {
dx = h->inv ? -motion->dx : motion->dx;
@@ -1032,16 +1032,20 @@ wsmouse_motion_sync(struct wsmouseinput
if (dy)
wsmouse_evq_put(evq, DELTA_Y_EV(input), dy);
if (motion->dz) {
+ dz = (input->flags & REVERSE_SCROLLING)
+ ? -motion->dz : motion->dz;
if (IS_TOUCHPAD(input))
- wsmouse_evq_put(evq, VSCROLL_EV, motion->dz);
+ wsmouse_evq_put(evq, VSCROLL_EV, dz);
else
- wsmouse_evq_put(evq, DELTA_Z_EV, motion->dz);
+ wsmouse_evq_put(evq, DELTA_Z_EV, dz);
}
if (motion->dw) {
+ dw = (input->flags & REVERSE_SCROLLING)
+ ? -motion->dw : motion->dw;
if (IS_TOUCHPAD(input))
- wsmouse_evq_put(evq, HSCROLL_EV, motion->dw);
+ wsmouse_evq_put(evq, HSCROLL_EV, dw);
else
- wsmouse_evq_put(evq, DELTA_W_EV, motion->dw);
+ wsmouse_evq_put(evq, DELTA_W_EV, dw);
}
}
if (motion->sync & SYNC_POSITION) {
@@ -1471,6 +1475,9 @@ wsmouse_get_params(struct device *sc,
case WSMOUSECFG_Y_INV:
params[i].value = input->filter.v.inv;
break;
+ case WSMOUSECFG_REVERSE_SCROLLING:
+ params[i].value = !!(input->flags & REVERSE_SCROLLING);
+ break;
case WSMOUSECFG_DX_MAX:
params[i].value = input->filter.h.dmax;
break;
@@ -1560,6 +1567,12 @@ wsmouse_set_params(struct device *sc,
break;
case WSMOUSECFG_Y_INV:
input->filter.v.inv = val;
+ break;
+ case WSMOUSECFG_REVERSE_SCROLLING:
+ if (val)
+ input->flags |= REVERSE_SCROLLING;
+ else
+ input->flags &= ~REVERSE_SCROLLING;
break;
case WSMOUSECFG_DX_MAX:
input->filter.h.dmax = val;
Index: sys/dev/wscons/wsmouseinput.h
===
RCS file: /cvs/src/sys/dev/wscons/wsmouseinput.h,v
audio: fix block size calculation
Currently the block size calculations are broken by design: the driver
provides a round_blocksize() function which must retrun a valid block
size in *bytes*. Unfortunately, since the driver doesn't know if it's
called for the play or for the record block size, it's mathematically
impossible to calculate the block size in all cases if play and record
number of channels are different. As a consequence, there are
half-working and weired hacks to find a usable block sizes.
The diff below addresses this by adding two new driver functions,
which are very simple to use:
set_blksz() - calculate and set the block size in *frames*, it's
necessarily common to play and recording directions no matter
the number of channels,
set_nblks() - calculate the number of blocks per buffer for the given
direction.
the diff below shows how to properly calculate the block size in
azalia and uaudio. The plan is to convert all drivers from
round_blocksize() to the new functions and to delete
round_blocksize().
Why is this important? besides for removing ugly (and risky) hacks, we
want all our drivers to support common block sizes in the 5ms-50ms
range. This would allow to implement switching between audio devices:
for instance, start playback on a USB device, unplug the cable and
continue on azalia.
OK?
Index: share/man/man9/audio.9
===
RCS file: /cvs/src/share/man/man9/audio.9,v
retrieving revision 1.27
diff -u -p -r1.27 audio.9
--- share/man/man9/audio.9 12 Mar 2019 08:18:34 - 1.27
+++ share/man/man9/audio.9 18 Aug 2019 05:28:35 -
@@ -82,6 +82,10 @@ struct audio_hw_if {
void (*)(void *), void *, struct audio_params *);
void(*copy_output)(void *hdl, size_t bytes);
void(*underrun)(void *hdl);
+ int (*set_blksz)(void *, int,
+ struct audio_params *, struct audio_params *, int);
+ int (*set_nblks)(void *, int, int,
+ struct audio_params *, int);
};
struct audio_params {
@@ -417,6 +421,56 @@ Drivers using bounce buffers for transfe
ring buffer and the device must implement this method to skip
one block from the audio ring buffer and transfer the
corresponding amount of silence to the device.
+.It Fn "int (*set_blksz)" "void *hdl" "int mode" \
+"struct audio_params *play" "struct audio_params *rec" "int blksz"
+This function is called to set the audio block size.
+.Fa mode
+is a combination of the
+.Dv AUMODE_RECORD
+and
+.Dv AUMODE_PLAY
+flags indicating the current mode set with the
+.Fn open
+function.
+The
+.Fa play
+and
+.Fa rec
+structures contain the current encoding set with the
+.Fn set_params
+function.
+.Fa blksz
+is the desired block size in frames.
+It may be adjusted to match hardware constraints.
+This function returns the adjusted block size.
+.It Fn "int (*set_nblks)" "void *hdl" "int dir" "int blksz" \
+"struct audio_params *params" "int nblks"
+This function is called to set the number of audio blocks in
+the ring buffer.
+.Fa dir
+is either the
+.Dv AUMODE_RECORD
+or the
+.Dv AUMODE_PLAY
+flag, indicating which ring buffer size is set.
+The
+.Fa params
+structure contains the encoding parameters set by the
+.Fn set_params
+method.
+.Fa blksz
+is the current block size in frames set with the
+.Fa set_params
+function.
+The
+.Fa params
+structure is the current encoding parameters, set with the
+.Fn set_params
+function.
+.Fa nblks
+is the desired number of blocks in the ring buffer.
+It may be lowered to at least two, to match hardware constraints.
+This function returns the adjusted number of blocks.
.El
.Pp
If the audio hardware is capable of input from more
Index: sys/dev/audio.c
===
RCS file: /cvs/src/sys/dev/audio.c,v
retrieving revision 1.180
diff -u -p -r1.180 audio.c
--- sys/dev/audio.c 17 Aug 2019 05:04:56 - 1.180
+++ sys/dev/audio.c 18 Aug 2019 05:28:36 -
@@ -193,6 +193,31 @@ audio_gcd(unsigned int a, unsigned int b
return a;
}
+/*
+ * Calculate the least block size (in frames) such that both the
+ * corresponding play and/or record block sizes (in bytes) are multiple
+ * of the given number of bytes.
+ */
+int
+audio_blksz_bytes(int mode,
+ struct audio_params *p, struct audio_params *r, int bytes)
+{
+ unsigned int np, nr;
+
+ if (mode & AUMODE_PLAY) {
+ np = bytes / audio_gcd(p->bps * p->channels, bytes);
+ if (!(mode & AUMODE_RECORD))
+ nr = np;
+ }
+ if (mode & AUMODE_RECORD) {
+ nr = bytes / audio_gcd(r->bps * r->channels, bytes);
+ if (!(mode & AUMODE_PLAY))
+ np = nr;
+ }
+
+ return nr * np / audio_gcd(nr, np);
+}
+
int
audio_buf_init(struct audio_softc *sc, struct audio_buf *buf, int dir)
{
@@ -625,11 +650,19 @@ audio_canstart(struct
