Re: [hackers] [sbase] ed: Don't use strlcpy() || Roberto E. Vargas Caballero
On Tue, Jan 10, 2017 at 08:56:46AM +0100, g...@suckless.org wrote:
> commit b95c8ed79e5d5322dd3c5c386c3acd62105ac116
> Author: Roberto E. Vargas Caballero
> AuthorDate: Tue Jan 10 08:46:48 2017 +0100
> Commit: Roberto E. Vargas Caballero
> CommitDate: Tue Jan 10 08:49:17 2017 +0100
>
> ed: Don't use strlcpy()
>
> All the buffers related to files have FILENAME_MAX size, so it is
> impossible
> to have any buffer overrun.
>
> diff --git a/ed.c b/ed.c
> index f579116..82fb784 100644
> --- a/ed.c
> +++ b/ed.c
> @@ -611,7 +611,7 @@ dowrite(const char *fname, int trunc)
> curln = line2;
> if (fclose(fp))
> error("input/output error");
> - if (strlcpy(savfname, fname, sizeof(savfname)) >= sizeof(savfname))
> + if (strcpy(savfname, fname, sizeof(savfname)) >= sizeof(savfname))
> error("file name too long");
I'm not sure if the strcpy check makes sense here. Is it intended?
To:
strcpy(savfname, fname, sizeof(savfname));
> modflag = 0;
> curln = line;
> @@ -743,8 +743,7 @@ getfname(char comm)
> } else {
> *bp = '\0';
> if (savfname[0] == '\0' || comm == 'e' || comm == 'f')
> - if (strlcpy(savfname, fname, sizeof(savfname)) >=
> sizeof(savfname))
> - error("file name too long");
> + strcpy(savfname, fname);
> return fname;
> }
>
>
--
Kind regards,
Hiltjo
[hackers] [sbase] ed: fix commit 2ccc1e8 || Roberto E. Vargas Caballero
commit 9ab1478f1eb6a9a355d394af3c0cfa69850245fe
Author: Roberto E. Vargas Caballero
AuthorDate: Tue Jan 10 11:28:58 2017 +0100
Commit: Roberto E. Vargas Caballero
CommitDate: Tue Jan 10 11:30:34 2017 +0100
ed: fix commit 2ccc1e8
The patch was wrong because the prototype of
strcpy is different to the prototype of strlcpy
diff --git a/ed.c b/ed.c
index 82fb784..4b28848 100644
--- a/ed.c
+++ b/ed.c
@@ -611,8 +611,7 @@ dowrite(const char *fname, int trunc)
curln = line2;
if (fclose(fp))
error("input/output error");
- if (strcpy(savfname, fname, sizeof(savfname)) >= sizeof(savfname))
- error("file name too long");
+ strcpy(savfname, fname);
modflag = 0;
curln = line;
}
Re: [hackers] [sbase] ed: Don't use strlcpy() || Roberto E. Vargas Caballero
On Tue, 10 Jan 2017 08:56:46 +0100 (CET) g...@suckless.org wrote: Hey Roberto, > All the buffers related to files have FILENAME_MAX size, so it is > impossible to have any buffer overrun. I consider this way of thinking harmful, because it involves assumptions about the code that are met in a different location. In case FILENAME_MAX is changed to some strange value, this entire building of thought breaks together. There's no reason not to use strlcpy, as it saves you from buffer overruns and other things. Nobody can possibly guarantee that some evil input has a non-null-terminated fname and we write to savfname without bounds. As Dimitris likes to say, programs spend 99% with I/O, so this "optimization" here won't make a difference. Premature optimization is the root of all evil, and given we get strlcpy() for free from libutil, I strongly suggest we keep the usage here. Cheers Laslo -- Laslo Hunhold
Re: [hackers] [sbase] [PATCH 1/5] Remove st != NULL checks from recursor functions
Hi Laslo On Tue, Dec 27, 2016 at 02:59:56PM +0100, Laslo Hunhold wrote: > On Wed, 14 Dec 2016 19:40:02 -0800 > Michael Forney wrote: > > Hey Michael, > > > In the description of 3111908b034c73673a2f079b2b13a88c18379baa, it > > says that the functions must be able to handle st being NULL, but > > recurse always passes a valid pointer. The only function that was > > ever passed NULL was rm(), but this was changed to go through recurse > > in 2f4ab527391135e651b256f8654b050ea4a48f3d, so now the checks are > > pointless. > > have you tested this patchset extensively? I hate to admit that the > recursor-subsystem is probably the most fragile part of sbase and > really need more feedback on these patches by more people (Silvan, have > you had the chance to test this?). Sadly, I have not :/ I just did some very basic testing using the following directory structure. testdir/ testdir/testdir2 testdir/testdir2/testB.txt testdir/testdir2/testA.txt testdir/link (symbolic link) testdir/test1.txt testdir/test2.txt I used the recursive options of tar, chgrp, chmod, chown, du and rm after applying Michael's patch on these files/directories and everything worked as expected. Ideally, somebody that uses sbase in a development system regularly should apply and test the patches further. Cheers, Silvan > Cheers > > Laslo > > -- > Laslo Hunhold >
Re: [hackers] [PATCH] simplify client moving on monitor count decrease
Hi Markus
On Wed, Jan 04, 2017 at 06:05:33PM +0100, Markus Teich wrote:
> ---
> dwm.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Looks good to me.
I have tested this patch yesterday and today in daily use and haven't
found any issues on my 2-monitor setup (yet).
Cheers,
Silvan
> diff --git a/dwm.c b/dwm.c
> index 4eba952..88d49d7 100644
> --- a/dwm.c
> +++ b/dwm.c
> @@ -1896,9 +1896,8 @@ updategeom(void)
> /* less monitors available nn < n */
> for (i = nn; i < n; i++) {
> for (m = mons; m && m->next; m = m->next);
> - while (m->clients) {
> + while ((c = m->clients)) {
> dirty = 1;
> - c = m->clients;
> m->clients = c->next;
> detachstack(c);
> c->mon = mons;
> --
> 2.10.2
>
>
Re: [hackers] [dwm] [PATCH 1/3] cleanup
Hi Markus
On Sat, Jan 07, 2017 at 05:21:28PM +0100, Markus Teich wrote:
> - unify multi-line expression alignment style.
> - unify multi-line function call alignment style.
> - simplify client moving on monitor count decrease.
> - clarify comment for focusin().
> - remove old confusing comment about input focus fix in focusmon(). The
> explanation is already in the old commit message, so no need to keep it in
> the
> code.
> - remove old comment describing even older state of the code in focus().
> - unify comment style.
> - break up some long lines.
> - fix some typos and grammar.
> ---
>
>
> Heyho,
>
> this patch contains some cleanup as Anselm anounced he plans to release a new
> version of dwm soon. Feel free to discuss on specific hunks as there are a few
> style unifications.
>
> --Markus
>
>
> LICENSE | 2 +-
> config.def.h | 2 +-
> dwm.c| 64
>
> 3 files changed, 32 insertions(+), 36 deletions(-)
>
> [...]
>
> for (m = mons; m && m->next; m = m->next);
> - while (m->clients) {
> + while ((c = m->clients)) {
> dirty = 1;
> - c = m->clients;
All changes are style and whitespace fixes except this one which I have
already reviewed so this patch looks good to me.
Cheers,
Silvan
> m->clients = c->next;
> detachstack(c);
> c->mon = mons;
> @@ -1913,8 +1910,7 @@ updategeom(void)
> free(unique);
> } else
> #endif /* XINERAMA */
> - /* default monitor setup */
> - {
> + { /* default monitor setup */
> if (!mons)
> mons = createmon();
> if (mons->mw != sw || mons->mh != sh) {
> @@ -1988,7 +1984,7 @@ updatesizehints(Client *c)
> } else
> c->maxa = c->mina = 0.0;
> c->isfixed = (c->maxw && c->minw && c->maxh && c->minh
> - && c->maxw == c->minw && c->maxh == c->minh);
> + && c->maxw == c->minw && c->maxh == c->minh);
> }
>
> void
> @@ -2082,8 +2078,8 @@ wintomon(Window w)
> }
>
> /* There's no way to check accesses to destroyed windows, thus those cases
> are
> - * ignored (especially on UnmapNotify's). Other types of errors call Xlibs
> - * default error handler, which may call exit. */
> + * ignored (especially on UnmapNotify's). Other types of errors call Xlibs
> + * default error handler, which may call exit. */
> int
> xerror(Display *dpy, XErrorEvent *ee)
> {
> @@ -2098,7 +2094,7 @@ xerror(Display *dpy, XErrorEvent *ee)
> || (ee->request_code == X_CopyArea && ee->error_code == BadDrawable))
> return 0;
> fprintf(stderr, "dwm: fatal error: request code=%d, error code=%d\n",
> - ee->request_code, ee->error_code);
> + ee->request_code, ee->error_code);
> return xerrorxlib(dpy, ee); /* may call exit */
> }
>
> --
> 2.10.2
>
>
Re: [hackers] [dwm] [PATCH 2/3] Button passthrough when client is not focused
Hi Markus
>From what I understand, in case that the client does not have focus
already, with this patch we call XGrabButton twice compared to before the
patch. I assume that corresponds to the advertised functionality.
One more comment below.
On Sat, Jan 07, 2017 at 05:21:29PM +0100, Markus Teich wrote:
> Before this change it is not possible to press a button in a client on the
> first
> click if the client is not yet focused. The first click on the button would
> only focus the client and a second click on the button is needed to activate
> it.
> This situation can occur when moving the mouse over a client (therefore
> focusing
> it) and then moving the focus to another client with keyboard shortcuts.
>
> After this commit the behavior is fixed and button presses on unfocused
> clients
> are passed to the client correctly.
> ---
>
>
> Heyho,
>
> this patch is the same as submitted previously. I just added it to this patch
> series for a better overview of the pending patches.
>
> --Markus
>
>
> dwm.c | 21 +++--
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/dwm.c b/dwm.c
> index 3f80b63..9c01d1a 100644
> --- a/dwm.c
> +++ b/dwm.c
> @@ -446,6 +446,8 @@ buttonpress(XEvent *e)
> click = ClkWinTitle;
> } else if ((c = wintoclient(ev->window))) {
> focus(c);
> + restack(selmon);
> + XAllowEvents(dpy, ReplayPointer, CurrentTime);
> click = ClkClientWin;
> }
> for (i = 0; i < LENGTH(buttons); i++)
> @@ -932,17 +934,16 @@ grabbuttons(Client *c, int focused)
> unsigned int i, j;
> unsigned int modifiers[] = { 0, LockMask, numlockmask,
> numlockmask|LockMask };
> XUngrabButton(dpy, AnyButton, AnyModifier, c->win);
> - if (focused) {
> - for (i = 0; i < LENGTH(buttons); i++)
> - if (buttons[i].click == ClkClientWin)
> - for (j = 0; j < LENGTH(modifiers); j++)
> - XGrabButton(dpy,
> buttons[i].button,
> - buttons[i].mask |
> modifiers[j],
> - c->win, False,
> BUTTONMASK,
> - GrabModeAsync,
> GrabModeSync, None, None);
> - } else
> + if (!focused)
> XGrabButton(dpy, AnyButton, AnyModifier, c->win, False,
> - BUTTONMASK, GrabModeAsync, GrabModeSync,
> None, None);
> + BUTTONMASK, GrabModeSync, GrabModeSync,
> None, None);
Here we pass "GrabModeSync" instead of "GrabModeAsync" as the "pointer
mode". According to https://tronche.com/gui/x/xlib/input/XGrabPointer.html
if the mode is "GrabModeSync",
"the state of the pointer, as seen by client applications, appears to
freeze, and the X server generates no further pointer events until
the grabbing client calls XAllowEvents() or until the pointer grab
is released."
Since we are calling XAllowEvents on buttonpress that should make sure
that only the focused client gets any pointer events, I assume?
I have been runnig with this patch for most of the day and haven't
encountered any issues.
Cheers,
Silvan
> + for (i = 0; i < LENGTH(buttons); i++)
> + if (buttons[i].click == ClkClientWin)
> + for (j = 0; j < LENGTH(modifiers); j++)
> + XGrabButton(dpy, buttons[i].button,
> + buttons[i].mask |
> modifiers[j],
> + c->win, False, BUTTONMASK,
> + GrabModeAsync,
> GrabModeSync, None, None);
> }
> }
>
> --
> 2.10.2
>
>
Re: [hackers] [dwm] [PATCH 2/3] Button passthrough when client is not focused
Silvan Jegen wrote:
> From what I understand, in case that the client does not have focus
> already, with this patch we call XGrabButton twice compared to before the
> patch. I assume that corresponds to the advertised functionality.
Heyho,
Not only twice but multiple times per button mapping and possible modifier mask.
I'm not sure why this is needed, but I tested it with several slight
modifications and none of them worked.
> On Sat, Jan 07, 2017 at 05:21:29PM +0100, Markus Teich wrote:
> > @@ -932,17 +934,16 @@ grabbuttons(Client *c, int focused)
> > unsigned int i, j;
> > unsigned int modifiers[] = { 0, LockMask, numlockmask,
> > numlockmask|LockMask };
> > XUngrabButton(dpy, AnyButton, AnyModifier, c->win);
> > - if (focused) {
> > - for (i = 0; i < LENGTH(buttons); i++)
> > - if (buttons[i].click == ClkClientWin)
> > - for (j = 0; j < LENGTH(modifiers); j++)
> > - XGrabButton(dpy,
> > buttons[i].button,
> > - buttons[i].mask |
> > modifiers[j],
> > - c->win, False,
> > BUTTONMASK,
> > - GrabModeAsync,
> > GrabModeSync, None, None);
> > - } else
> > + if (!focused)
> > XGrabButton(dpy, AnyButton, AnyModifier, c->win, False,
> > - BUTTONMASK, GrabModeAsync, GrabModeSync,
> > None, None);
> > + BUTTONMASK, GrabModeSync, GrabModeSync,
> > None, None);
>
> Here we pass "GrabModeSync" instead of "GrabModeAsync" as the "pointer
> mode". According to https://tronche.com/gui/x/xlib/input/XGrabPointer.html
> if the mode is "GrabModeSync",
>
> "the state of the pointer, as seen by client applications, appears to
> freeze, and the X server generates no further pointer events until
> the grabbing client calls XAllowEvents() or until the pointer grab
> is released."
>
> Since we are calling XAllowEvents on buttonpress that should make sure
> that only the focused client gets any pointer events, I assume?
In my understanding the point of the XAllowEvents call is to replay the event on
the client under the cursor after it has been focused.
> I have been runnig with this patch for most of the day and haven't
> encountered any issues.
>
> > + for (i = 0; i < LENGTH(buttons); i++)
> > + if (buttons[i].click == ClkClientWin)
> > + for (j = 0; j < LENGTH(modifiers); j++)
> > + XGrabButton(dpy, buttons[i].button,
> > + buttons[i].mask |
> > modifiers[j],
> > + c->win, False, BUTTONMASK,
> > + GrabModeAsync,
> > GrabModeSync, None, None);
> > }
> > }
Disclaimer: This patch is a bit of a grey area for my understanding of the
grabbing mechanics. While preparing the user-patch for unsloppy focusing I
noticed this bit from Erics patch is used to fix a bug which is also present in
vanilla dwm. It's a little improbable to hit this bug in vanilla dwm, which is
why it probably hasn't been discovered previously. So I just took the bug-fix
part from Erics patch, applied it to vanilla dwm, also tested it for a while and
it worked as I intend it to. However I would be very thankful if someone with
more knowledge of the grabbing semantics could explain why exactly the bug
occurs before this patch, why the changes in this patch fix that behavior and
ultimately approve this patch for merging.
--Markus
Re: [hackers] [sbase] Revert "ed: remove double free in join()" || Roberto E. Vargas Caballero
Hello Roberto, The trouble with reverting my commit is that readding the double free completely crashes ed if more than one join is performed. I think this patch (which also reverts back to having no double free) should handle your concern via blocking signal handling until the join is finished. Thomas -- Thomas Mannay 0001-ed-ignore-signals-inside-of-join.patch Description: Binary data
