This repository was archived by the owner on Oct 20, 2023. It is now read-only.
Don't grow logevent buf indefinitely #1
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As found by a bot ( http://www.misfix.org, https://github.com/ka7/misspell_fixer ).
A couple of the functions for which I was already turning off the type check for old Visual Studio turn out to also need it turning off for MinGW.
It's not on the default list of important system 'known DLLs' stored at HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\KnownDLLs (see https://isc.sans.edu/forums/diary/DLL+hijacking+vulnerabilities/9445/ ) which apparently makes it exempt from Windows's standard DLL hijacking defence, i.e. if an executable links against it in the normal way then that executable will be vulnerable to DLL hijacking from a file called winmm.dll in the same directory as it. The solution is to load it dynamically _after_ we've locked down our DLL search path, which fortunately PuTTY's code base is well used to doing already for other DLLs.
The printing functions are split between winspool.drv and spoolss.dll in a really weird way (who would have guessed that OpenPrinter and ClosePrinter don't live in the same dynamic library?!), but _neither_ of those counts as a system 'known DLL', so linking against either one of these at load time is again a potential DLL hijacking vector.
This too is not in the list of known DLLs on Windows 10. I don't know of any actual viable hijacking attack based on it, which according to my reading of MSDN (specifically, a rather vague hint in https://msdn.microsoft.com/library/ff919712) _may_ be because we mention the common controls assembly in our application manifest; but better safe than sorry. Now the entire list of remaining DLLs that PuTTY links against at load time is a subset of the Win10 known DLLs list, so that _should_ mean that everything we load before we've deployed our own defence (SetDefaultDllDirectories) is defended against for us by Windows itself.
The rewritten bugs2html.py in the wishlist repository no longer needs me to manually maintain a mapping between releases and version control - and the one thing I forgot was to remove the reminder in the release checklist telling me to keep that mapping up to date :-)
This was accidentally disabled by 73039b7, causing a regression in ability to type characters outside of the current Windows code page.
This prevented compilation with Gtk 2.
This fixes compilation with Gtk 2 with -Werror. Problem introduced by 6422197.
Thanks to Brian K. White for spotting this straight-up syntax error of a missing ), in the regex handling the special case of &splitlines when it findss a word in its input string too long to fit in the specified output line width. Apparently in all my own uses of &splitline I'd never exercised that special-case code path before.
This allows me to remove HTML Help Workshop completely from my build dependencies, and good riddance!
It had the constant value 1 everywhere that it was read.
Nothing was using its return value anyway.
Nothing used their return values anyway. At least, not after the previous commit.
It's redundant with back->connected(): only the SSH backend has a receive function that can ever return 0, and whenever ssh_receive returns 0 it has called ssh_do_close, which will cause future calls to ssh_connected also to return 0. Similarly, all backend closing functions ensure that future calls to their connected function will return 0.
Nothing now uses its return value anyway.
plug_receive() and plug_closing() return 0 or 1 depending on whether they think the main connection has closed. It is not appropriate, as handle_gotdata and handle_socket_unfreeze did, to treat them as returning a backlog. In fact, plugs are unusual in PuTTY in not reporting a backlog, but just calling into the socket to freeze and unfreeze it as required.
Nothing was paying attention to their return values any more anyway.
When making select_result() return void (a2fb1d9), I removed a "return" at the end of the FD_CLOSE case, causing a fallthrough into FD_ACCEPT with hilarious (segfaulting) consequences. Re-instate the "return".
More fallout from 6422197.
- I haven't heard of OpenSSH/OpenSSL mismatches being a common problem for a long time. Specific advice about OpenSSH 3.1/3.4 seems unlikely to be useful these days. - "Incorrect MAC received on packet" doesn't seem to be a common problem these days, and if anyone encounters it, the words in the "Errors" bit of the docs seem adequate without a FAQ entry as well.
While it's still true, the link to Winsock 2 is dead, our standard release builds don't run on Win95 any more, and it's certainly not frequently asked.
Patch due to Brian K. White; we now condition our own declaration of DLL_DIRECTORY_COOKIE on whether the toolchain's headers had #defined it already, rather than trying to guess the answer to that from version-number macros. (Apparently everything that defines DLL_DIRECTORY_COOKIE does it by does seem to work in all my tests.)
stdint.h is one of the set of standard C headers that has more to do with the compiler than the library, and hence, clang provides its own version of it, even when you're using it in clang-cl mode with Visual Studio headers, and even when those headers are old enough not to have a stdint.h of their own. So in clang builds, including stdint.h is always the right way to get uintptr_t defined.
This lays some groundwork for making PuTTY's cut and paste handling more flexible in the area of which clipboard(s) it reads and writes, if more than one is available on the system. I've introduced a system of list macros which define an enumeration of integer clipboard ids, some defined centrally in putty.h (at present just a CLIP_NULL which never has any text in it, because that seems like the sort of thing that will come in useful for configuring a given copy or paste UI action to be ignored) and some defined per platform. All the front end functions that copy and paste take a clipboard id, and the Terminal structure is now configured at startup to tell it which clipboard id it should paste from on a mouse click, and which it should copy from on a selection. However, I haven't actually added _real_ support for multiple X11 clipboards, in that the Unix front end supports a single CLIP_SYSTEM regardless of whether it's in OS X or GTK mode. So this is currently a NFC refactoring which does nothing but prepare the way for real changes to come.
All the data fields referring to the selection in 'struct gui_data' have been pulled out into a separate structure of which there are now multiple instances, and I've plumbed through what should be the right pointers and integer ids to everywhere they should go. So now the GTK front end defines CLIP_PRIMARY and CLIP_CLIPBOARD in place of the temporary cop-out CLIP_SYSTEM from the previous commit, and copying and pasting can be done via either one. The defaults should be the same as before, except that now the non-Mac versions of the GtkApplication front ends will access CLIP_PRIMARY in response to most actions but the 'Paste' menu item will paste from CLIP_CLIPBOARD. (That's mostly just as a demonstration that accessing multiple clipboards even works.)
This stores the last text selected in _this_ terminal, regardless of whether any other application has since taken back whatever system clipboard we also copied it to. It's written unconditionally whenever text is selected in terminal.c. The main purpose of this will be that it's also the place that you can go and find the data you need to write to a system clipboard in response to an explicit Copy operation. But it can also act as a data source for pastes in its own right, so you can use it to implement an intra-window private extra clipboard if that's useful. (OS X Terminal has one of those, so _someone_ at least seems to like the idea.)
This makes space in the Selection panel (at least on Windows; it wasn't overfull on Unix) to add a new set of config options controlling the mapping of UI actions to clipboards. (A possible future advantage of having spare space in this new Words panel is that there's room to add controls for context-sensitive special-casing, e.g. I'd quite like ':' to be treated differently when it appears as part of "http://".)
On all platforms, you can now configure which clipboard the mouse pastes from, which clipboard Ctrl-Ins and Shift-Ins access, and which Ctrl-Shift-C and Ctrl-Shift-V access. In each case, the options are: - nothing at all - a clipboard which is implicitly written by the act of mouse selection (the PRIMARY selection on X, CLIP_LOCAL everywhere else) - the standard clipboard written by explicit copy/paste UI actions (CLIPBOARD on X, the unique system clipboard elsewhere). Also, you can control whether selecting text with the mouse _also_ writes to the explicitly accessed clipboard. The wording of the various messages changes between platforms, but the basic UI shape is the same everywhere.
I've done the general clipboard revamp, and also, since I added
Ctrl-Shift-{C,V} as a new pair of UI actions for copy and paste, I've
also fulfilled the requirement that there should be some method of
non-menu-based pasting that doesn't depend on a middle mouse button or
an Ins key.
I think the list of OS X missing features is now down to details of
the OS X GTK port _itself_, as opposed to structural issues in the
general code base.
This required me to turn the drop-lists into combo boxes and add an extra string-typed Conf setting alongside each enumerated value.
I had a segfault on OS X today at Pterm.app shutdown. I wasn't able to reproduce it in a debugger, but the cause seemed to be that clipboard_clear called term_deselect (this was from before the patch series that renamed that function) when inst->term was already NULL. This must be because a clipboard_data_instance outlived its associated inst->term, and quite likely its associated inst as well. But we can't free those structures when a gui_data is freed, because GTK callbacks will still depend on them; so instead we must have each gui_data keep a list of active cdis pointing at it, and then at destruction time, walk along the list nulling out each one's pointer to part of itself.
The omission of this call caused a GTK assertion failure when the GApplication's use count went negative after two releases and only one hold.
The gtkapp.c menu now has a Copy as well as Paste option; those menu items, as well as the corresponding ones on the context menu and Copy All, now address sets of clipboards parametrised between OS X and ordinary GTK in unix.h. Also I've tweaked the wording of the context-menu items to not use the X-specific terminology "CLIPBOARD" on OS X.
Oh dear. I have no excuse.
That's what I get for not testing on all platforms before I push. Forgot that, since OS X GTK mimics X11 GTK closely enough to still use the name "CLIPBOARD" for the unique system clipboard, I had left this code base's internal name for it as CLIP_CLIPBOARD and not the CLIP_SYSTEM I used on Windows.
This fixes the problem I'd previously noticed, that if you don't configure the "Command key acts as Meta" setting, then keystrokes like Command-Q which _ought_ to function as accelerators for the application menu bar don't. Turns out that this was for the totally obvious reason: the keyboard event was still being processed by gtkwin.c's key_event() and translated via the GTK IM into ordinary keyboard input. If instead I return FALSE from key_event on detecting that a key event has a non-Meta-configured Command modifier, then it will go to the next- level key-event handler inside GTK itself which implements the menu accelerator behaviour. Another problem ticked off the OS X checklist.
Now that they actually _work_, it seems more useful to start putting them in on purpose.
Just to avoid an endless proliferation of functions too small to see, I've arranged an enumeration of action ids and a single app_menu_action function on the receiving end, and in gtkapp.c, a list macro that means I at least don't have to define the tiny callback functions and the GActionEntry records by hand and keep them in sync.
This still isn't complete: I also need to add the variable collections of things like mid-session special commands and saved session names, and also I need to try to grey out menu items when they're not applicable. But it's a start.
I've filled in the results of some not-entirely-conclusive investigation into the trackpad scrolling issue, some thoughts on resizing, and reordered the items into what currently seems the most sensible order to me.
By default, the program still builds on Linux to a stub that just prints 'nothing to see here'. But if you compile with -DTEST_COMPILE_ON_LINUX, it compiles to a program that still doesn't do anything _actually_ useful, but goes through all the same motions that real osxlaunch would go through, until the final execv(2) fails because of course it's not _really_ living in an application bundle directory of the right shape. That allows me to run all the setup code under the debugging tools I'm most used to, in my preferred environment. (Same rationale as having puttyapp / ptermapp build for Linux too.)
Now I can compile with -DDEBUG_OSXLAUNCH and see exactly what the program is doing, if I suspect it of misbehaviour.
I've been having intermittent segfaults in this launcher program, and by means of the new TEST_COMPILE_ON_LINUX facility introduced by commit eef8cac, I ran it under valgrind which helpfully pointed out several pointers between linked-list nodes which I'd been relying on OS memory allocation to happen to have zeroed for me.
It actually doesn't seem to be necessary: running 'otool -L' on the
real binary in the application bundle (Pterm-bin or PuTTY-bin) lists a
lot of paths starting with "@executable_path/../Resources/", which I
take to mean that the application is already set up to automatically
load the GTK shared libraries out of its own bundle directory, without
me having to give it the extra hint of DYLD_LIBRARY_PATH.
Moreover, I just got round to upgrading my Mac to High Sierra, and now
the version of osxlaunch _with_ DYLD_LIBRARY_PATH is causing a crash
at program load time, when the libpng in the MacOS system library
directory tries to use the libz in the application bundle and finds
that it doesn't provide an entry point it was expecting
('inflateValidate'). I could try to fix that by updating the libz
version in my OS X PuTTY build environment, but that seems to me to
set a precedent of running to keep up with any further dependencies
the system libraries happen to acquire in later releases. Better to
reset DYLD_LIBRARY_PATH so that the system libpng will load the system
libz and not get confused in the first place.
A clang too old to have __attribute__((target)) will not manage to compile the clang-style hardware-accelerated functions, so it shouldn't try.
gtk_application_set_accels_for_action() is new in Gtk 3.12, but (e.g.) Ubuntu 14.04 LTS still ships with Gtk 3.10. On the other hand, the function I've used instead, gtk_application_add_accelerator(), is deprecated from Gtk 3.14 onwards, indicating that it will disappear in some future version, so I've left the newer code in against that day.
The PuTTY GUIs (Unix and Windows) maintain an in-memory event log
for display to users as they request. This uses ints for tracking
eventlog size, which is subject to memory exhaustion and (given
enough heap space) overflow attacks by servers (via, e.g., constant
rekeying).
Also a bounded log is more user-friendly. It is rare to want more
than the initial logging and the logging from a few recent rekey
events.
The Windows fix has been tested using Dr. Memory as a valgrind
substitute. No errors corresponding to the affected code showed up.
The Dr. Memory results.txt was split into a file per-error and then
grep Error $(grep -l windlg *)|cut -d: -f3-|sort |uniq -c
was used to compare. Differences arose from different usage of the GUI,
but no error could be traced to the code modified in this commit.
The Unix fix has been tested using valgrind. We don't destroy the
eventlog_stuff eventlog arrays, so we can't be entirely sure that we
don't leak more than we did before, but from code inspection it looks
like we don't (and anyways, if we leaked as much as before, just without
the integer overflow, well, that's still an improvement).
Author
|
This repository seems to be way behind the upstream master, closing PR. |
rtsp
pushed a commit
to rtsp7/putty
that referenced
this pull request
Sep 7, 2021
When do_paint breaks up a line of terminal text into contiguous runs of characters to treat the same, one of the criteria it uses is, 'Does this character even need redrawing? (or is it already displayed correctly from the previous redraw?)' When we encounter a character that matches its previous value, we end the previous run of characters, so that we can skip the one we've just encountered. That check was not taking account of the 'truecolour' field of the termchar it was checking. So it would sometimes falsely believe the character to be equivalent to its previously drawn value, even when in fact it was not, and hence insert a run break, anticipating that the previous character needed drawing and the current one did not. This didn't cause a _wrong_ redraw, because there's a separate loop further on which re-checks whether to actually draw things, which didn't make the same error. So the character that loop github#1 thought didn't need a redraw, loop github#2 knew _did_ need a redraw, and hence, everything did get redrawn. But by the time loop github#2 is running, it's too late to change the run boundaries. So everything does get redrawn, but in much smaller chunks than it could have been. The net effect was that if the screen was filled with text displayed in true colour, and you changed it to the _same_ text in a different colour, then the whole terminal would be redrawn in one-character increments instead of the usual behaviour of folding together runs that can be drawn in one go. Thanks to Bradley Smith for debugging this very confusing issue!
rtsp
pushed a commit
to rtsp7/putty
that referenced
this pull request
Dec 28, 2021
Now, we always try an initial CONNECT request with no auth at all, and wait for the proxy to reject it before sending a second try with auth. That way, we can wait to see what _kind_ of authentication the proxy requests, which will enable us to support something more secure than Basic, such as HTTP Digest. (I mean, it would _work_ to try Basic in request github#1 and then retrying with Digest in github#2 when the proxy asks for it. But if the aim of using Digest is to avoid sending the password in cleartext, it defeats the entire purpose to have sent it in cleartext anyway by the time you realise the server is prepared to do something better!)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
11 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PuTTY GUIs (Unix and Windows) maintain an in-memory event log for display to users as they request. This uses ints for tracking eventlog size, which is subject to memory exhaustion and (given
enough heap space) overflow attacks by servers (via, e.g., constant rekeying).
Also a bounded log is more user-friendly. It is rare to want more than the initial logging and the logging from a few recent rekey events.
This patch retains the first 128 log events, and thereafter maintains a circular buffer of up to 128 additional events.