simon-git: putty (main): Simon Tatham
Commits to Tartarus hosted VCS
tartarus-commits at lists.tartarus.org
Wed Sep 14 16:13:31 BST 2022
TL;DR:
7339e00f windows/utils/registry.c: allow opening reg keys RO.
6cf6682c Rewrite some manual char-buffer-handling code.
20f818af Rename 'ret' variables passed from allocation to return.
c780cffb Remove a pointless allocation.
Repository: https://git.tartarus.org/simon/putty.git
On the web: https://git.tartarus.org/?p=simon/putty.git
Branch updated: main
Committer: Simon Tatham <anakin at pobox.com>
Date: 2022-09-14 16:13:31
commit 7339e00f4ac8a0e5f0e101d2a0effa18ef794034
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=7339e00f4ac8a0e5f0e101d2a0effa18ef794034;hp=6a1eba054f892465c2e12d30132eb31132b8dc04
Author: Simon Tatham <anakin at pobox.com>
Date: Wed Sep 14 16:04:14 2022 +0100
windows/utils/registry.c: allow opening reg keys RO.
These handy wrappers on the verbose underlying Win32 registry API have
to lose some expressiveness, and one thing they lost was the ability
to open a registry key without asking for both read and write access.
This meant they couldn't be used for accessing keys not owned by the
calling user.
So far, I've only used them for accessing PuTTY's own saved data,
which means that hasn't been a problem. But I want to use them
elsewhere in an upcoming commit, so I need to fix that.
The obvious thing would be to change the meaning of the existing
'create' boolean flag so that if it's false, we also don't request
write access. The rationale would be that you're either reading or
writing, and if you're writing you want both RW access and to create
keys that don't already exist. But in fact that's not true: you do
want to set create==false and have write access in the case where
you're _deleting_ things from the key (or the whole key). So we really
do need three ways to call the wrapper function.
Rather than add another boolean field to every call site or mess about
with an 'access type' enum, I've taken an in-between route: the
underlying open_regkey_fn *function* takes a 'create' and a 'write'
flag, but at call sites, it's wrapped with a macro anyway (to append
NULL to the variadic argument list), so I've just made three macros
whose names request different access. That makes call sites marginally
_less_ verbose, while still
windows/platform.h | 10 +++++++---
windows/storage.c | 39 +++++++++++++++++++--------------------
windows/utils/registry.c | 10 +++++-----
3 files changed, 31 insertions(+), 28 deletions(-)
commit 6cf6682c549a381e38e1895cb1c2d039cec6334f
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=6cf6682c549a381e38e1895cb1c2d039cec6334f;hp=7339e00f4ac8a0e5f0e101d2a0effa18ef794034
Author: Simon Tatham <anakin at pobox.com>
Date: Tue Sep 13 15:00:26 2022 +0100
Rewrite some manual char-buffer-handling code.
In the course of recent refactorings I noticed a couple of cases where
we were doing old-fashioned preallocation of a char array with some
conservative maximum size, then writing into it via *p++ or similar
and hoping we got the calculation right.
Now we have strbuf and dupcat, so we shouldn't ever have to do that.
Fixed as many cases as I could find by searching for allocations of
the form 'snewn(foo, char)'.
Particularly worth a mention was the Windows GSSAPI setup code, which
was directly using the Win32 Registry API, and looks much more legible
using the windows/utils/registry.c wrappers. (But that was why I had
to enhance them in the previous commit so as to be able to open
registry keys read-only: without that, the open operation would
actually fail on this key, which is not user-writable.)
Also unix/askpass.c, which was doing a careful reallocation of its
buffer to avoid secrets being left behind in the vacated memory -
which is now just a matter of ensuring we called strbuf_new_nm().
otherbackends/telnet.c | 119 +++++++++++++++++++++----------------------------
pscp.c | 25 ++++-------
settings.c | 60 +++++++------------------
ssh/scpserver.c | 17 +++----
terminal/terminal.c | 13 +++---
terminal/terminal.h | 3 +-
unix/askpass.c | 63 ++++++++------------------
unix/dialog.c | 4 +-
unix/pty.c | 3 +-
unix/storage.c | 12 +++--
windows/controls.c | 31 +++++--------
windows/dialog.c | 30 ++++---------
windows/gss.c | 87 ++++++++++++++++--------------------
13 files changed, 175 insertions(+), 292 deletions(-)
commit 20f818af12012771165ac039dfbfcb3ce692e474
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=20f818af12012771165ac039dfbfcb3ce692e474;hp=6cf6682c549a381e38e1895cb1c2d039cec6334f
Author: Simon Tatham <anakin at pobox.com>
Date: Tue Sep 13 14:53:36 2022 +0100
Rename 'ret' variables passed from allocation to return.
I mentioned recently (in commit 9e7d4c53d80b6eb) message that I'm no
longer fond of the variable name 'ret', because it's used in two quite
different contexts: it's the return value from a subroutine you just
called (e.g. 'int ret = read(fd, buf, len);' and then check for error
or EOF), or it's the value you're preparing to return from the
_containing_ routine (maybe by assigning it a default value and then
conditionally modifying it, or by starting at NULL and reallocating,
or setting it just before using the 'goto out' cleanup idiom). In the
past I've occasionally made mistakes by forgetting which meaning the
variable had, or accidentally conflating both uses.
If all else fails, I now prefer 'retd' (short for 'returned') in the
former situation, and 'toret' (obviously, the value 'to return') in
the latter case. But even better is to pick a name that actually says
something more specific about what the thing actually is.
One particular bad habit throughout this codebase is to have a set of
functions that deal with some object type (say 'Foo'), all *but one*
of which take a 'Foo *foo' parameter, but the foo_new() function
starts with 'Foo *ret = snew(Foo)'. If all the rest of them think the
canonical name for the ambient Foo is 'foo', so should foo_new()!
So here's a no-brainer start on cutting down on the uses of 'ret': I
looked for all the cases where it was being assigned the result of an
allocation, and renamed the variable to be a description of the thing
being allocated. In the case of a new() function belonging to a
family, I picked the same name as the rest of the functions in its own
family, for consistency. In other cases I picked something sensible.
One case where it _does_ make sense not to use your usual name for the
variable type is when you're cloning an existing object. In that case,
_neither_ of the Foo objects involved should be called 'foo', because
it's ambiguous! They should be named so you can see which is which. In
the two cases I found here, I've called them 'orig' and 'copy'.
As in the previous refactoring, many thanks to clang-rename for the
help.
crypto/rsa.c | 8 +-
dialog.c | 14 +-
import.c | 116 +++++++--------
pageant.c | 16 +-
ssh/crc-attack-detector.c | 8 +-
ssh/sftp.c | 42 +++---
sshpubk.c | 22 +--
unix/network.c | 346 +++++++++++++++++++++----------------------
unix/printing.c | 12 +-
unix/sftp.c | 51 +++----
unix/utils/filename.c | 6 +-
utils/tree234.c | 10 +-
windows/named-pipe-server.c | 34 ++---
windows/network.c | 218 +++++++++++++--------------
windows/printing.c | 42 +++---
windows/sftp.c | 50 +++----
windows/storage.c | 22 +--
windows/utils/filename.c | 6 +-
windows/utils/request_file.c | 6 +-
19 files changed, 513 insertions(+), 516 deletions(-)
commit c780cffb7a6f1c972f5629e6e4976c5616ca0df3
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=c780cffb7a6f1c972f5629e6e4976c5616ca0df3;hp=20f818af12012771165ac039dfbfcb3ce692e474
Author: Simon Tatham <anakin at pobox.com>
Date: Tue Sep 13 15:48:11 2022 +0100
Remove a pointless allocation.
The char buffer 'blob' allocated in read_blob was never actually used
for anything, so there was no need to allocate it - and also no need
for the assert() just before it, which was added in commit
63a58759b5c0c11 to be extra safe against integer overflow when
computing the buffer size.
I feel a bit silly for having added that assertion in the first place
rather than removing the allocation it was protecting! It was
unnecessary even then, and I completely failed to notice :-)
sshpubk.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
More information about the tartarus-commits
mailing list