from:"Daniel Watzinger"

Re: pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

2023-03-14 Thread Daniel Watzinger

I'm sorry I couldn't contribute to the discussion in time. The fix of the
fstat() Win32 port looks good to me. I agree that there's a need for
multiple fseek() ports to address the shortcomings of the MSVC
functionality.

The documentation event states that "on devices incapable of seeking, the
return value is undefined". A simple wrapper using GetFileType() or the new
fstat(), to filter non-seekable devices before delegation, will probably
suffice.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fseek-fseeki64?view=msvc-170

Regarding test automation and regression testing, there's a programmatic
way to simulate how the "pipe operator" of cmd.exe and other shells works
using CreateProcess and manual "piping" by means of various WinAPI
functionality. This is actually how the bug was discovered in the first
case. However, existing tests are probably platform-agnostic.

--
Daniel

On Tue, Mar 14, 2023 at 12:41 AM Michael Paquier 
wrote:

> On Mon, Mar 13, 2023 at 05:49:41PM +0100, Juan José Santamaría Flecha
> wrote:
> > WFM, making fseek() behaviour more resilient seems like a good
> improvement
> > overall.
>
> I have not looked in details, but my guess would be to add a
> win32seek.c similar to win32stat.c with a port of fseek() that's more
> resilient to the definitions in POSIX.
>
> > Should we open a new thread to make that part more visible?
>
> Yes, perhaps it makes sense to do so to attract the correct audience,
> There may be a few things we are missing.
>
> When it comes to pg_dump, both fixes are required, still it seems to
> me that adjusting the fstat() port and the fseek() ports are two
> different bugs, as they influence different parts of the code base
> when taken individually (aka this fseek() port for WIN32 would need
> fstat() to properly detect a pipe, as far as I understand).
>
> Meanwhile, I'll go apply and backpatch 0001 to fix the first bug at
> hand with the fstat() port, if there are no objections.
> --
> Michael
>

pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

2022-12-18 Thread Daniel Watzinger


Well, this is embarassing. Sorry for the inconvenience. Some part
of my company's network infrastruture must have mangled the attachment.
Both mails were sent using a combination of git format-patch 
and git send-email. However, as this is my first foray into this 
email-based workflow, I won't rule out a failure on my part. Bear
with me and let's try again. diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7f7a0f1ce7..684db4a17a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3838,6 +3838,18 @@ bool
 checkSeek(FILE *fp)
 {
 	pgoff_t		tpos;
+	struct stat	st;
+
+	/* Check if this is a terminal descriptor */
+	if (isatty(fileno(fp))) {
+		return false;
+	}
+
+	/* Check if this is an unseekable character special device or pipe */
+	if ((fstat(fileno(fp), &st) == 0) && (S_ISCHR(st.st_mode)
+		|| S_ISFIFO(st.st_mode))) {
+		return false;
+	}
 
 	/* Check that ftello works on this file */
 	tpos = ftello(fp);
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index 296905bc6c..892d0bb401 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -329,6 +329,12 @@ extern int	_pglstat64(const char *name, struct stat *buf);
 #ifndef S_ISREG
 #define S_ISREG(m) (((m) & S_IFMT) == S_IFREG)
 #endif
+#ifndef S_ISFIFO
+#define S_ISFIFO(m) (((m) & S_IFMT) == _S_IFIFO)
+#endif
+#ifndef S_ISCHR
+#define S_ISCHR(m) (((m) & S_IFMT) == S_IFCHR)
+#endif
 
 /*
  * In order for lstat() to be able to report junction points as symlinks, we
diff --git a/src/port/win32stat.c b/src/port/win32stat.c
index e6553e4030..7ab0983a74 100644
--- a/src/port/win32stat.c
+++ b/src/port/win32stat.c
@@ -256,35 +256,65 @@ _pgstat64(const char *name, struct stat *buf)
 int
 _pgfstat64(int fileno, struct stat *buf)
 {
-	HANDLE		hFile = (HANDLE) _get_osfhandle(fileno);
-	BY_HANDLE_FILE_INFORMATION fiData;
+	HANDLE			hFile = (HANDLE) _get_osfhandle(fileno);
+	DWORD			fileType = FILE_TYPE_UNKNOWN;
+	DWORD			lastError;
+	unsigned short	st_mode;
 
-	if (hFile == INVALID_HANDLE_VALUE || buf == NULL)
+	if (hFile == INVALID_HANDLE_VALUE || hFile == (HANDLE)-2 || buf == NULL)
 	{
 		errno = EINVAL;
 		return -1;
 	}
 
-	/*
-	 * Check if the fileno is a data stream.  If so, unless it has been
-	 * redirected to a file, getting information through its HANDLE will fail,
-	 * so emulate its stat information in the most appropriate way and return
-	 * it instead.
+	fileType = GetFileType(hFile);
+	lastError = GetLastError();
+
+	/* 
+	 * Invoke GetLastError in order to distinguish between a "valid"
+	 * return of FILE_TYPE_UNKNOWN and its return due to a calling error.
+	 * In case of success, GetLastError returns NO_ERROR.
 	 */
-	if ((fileno == _fileno(stdin) ||
-		 fileno == _fileno(stdout) ||
-		 fileno == _fileno(stderr)) &&
-		!GetFileInformationByHandle(hFile, &fiData))
+	if (fileType == FILE_TYPE_UNKNOWN && lastError != NO_ERROR) {
+		_dosmaperr(lastError);
+		return -1;
+	}
+
+	switch (fileType)
 	{
-		memset(buf, 0, sizeof(*buf));
-		buf->st_mode = _S_IFCHR;
-		buf->st_dev = fileno;
-		buf->st_rdev = fileno;
-		buf->st_nlink = 1;
-		return 0;
+		/* The specified file is a disk file */
+		case FILE_TYPE_DISK:
+			return fileinfo_to_stat(hFile, buf);
+		/*
+		 * The specified file is a socket,
+		 * a named pipe, or an anonymous pipe.
+		 */
+		case FILE_TYPE_PIPE:
+			st_mode = _S_IFIFO;
+			break;
+		/* 
+		 * The specified file is a character file,
+		 * typically an LPT device or a console
+		 */
+		case FILE_TYPE_CHAR:
+		/* 
+		 * Unused flag and unknown file type
+		 */
+		case FILE_TYPE_REMOTE:
+		case FILE_TYPE_UNKNOWN:
+			st_mode = _S_IFCHR;
+			break;
+		default:
+			errno = EINVAL;
+			return -1;
 	}
 
-	return fileinfo_to_stat(hFile, buf);
+	memset(buf, 0, sizeof(*buf));
+	buf->st_mode = st_mode;
+	buf->st_dev = fileno;
+	buf->st_rdev = fileno;
+	buf->st_nlink = 1;
+	return 0;
 }
 
 #endif			/* WIN32 */

base-commit: e52f8b301ed54aac5162b185b43f5f1e44b6b17e

pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

2022-12-16 Thread Daniel Watzinger

Hi there,

first-time contributor here. I certainly hope I got the patch
creation and email workflow right. Let me know if anything can be
improved as I`m eager to learn. Regression tests (check) were 
successful on native Win32 MSVC as well as Debian. Here comes the 
patch and corresponding commit text.

During archive initialization pg_backup_custom.c determines if the file
pointer it should read from or write to is seekable. pg_dump
uses this information to rewrite the custom output format's TOC
enriched with known offsets into the archive on close. pg_restore uses
seeking to speed up file operations when searching for specific
blocks within the archive.

The seekable property of a file pointer is currently checked by
invoking ftello and subsequently fseeko. Both calls succeed
on Windows platforms if the underlying file descriptor represents a
terminal handle or an anonymous or named pipe. Obviously, these type
of devices do not support seeking. In the case of pg_dump, this
leads to the TOC being appended to the end of the output when attempting
to rewrite known offsets. Furthermore, pg_restore may try to seek to
known file offsets if the custom format archive's TOC supports it
and subsequently fails to locate blocks.

This commit improves the detection of the seekable property by checking
a descriptor's file type (st_mode) and filtering character special
devices and pipes. The current customized implementation of fstat on
Windows platforms (_pgfstat64) erroneously marks terminal and pipe
handles as regular files (_S_IFREG). This was improved on by
utilizing WinAPI functionality (GetFileType) to correctly distinguish
and flag descriptors based on their native OS handle's file type.

Daniel

---
 src/bin/pg_dump/pg_backup_archiver.c | 12 +
 src/include/port/win32_port.h|  6 +++
 src/port/win32stat.c | 68 
 3 files changed, 67 insertions(+), 19 deletions(-)
v'ß+ZþÊÜý¸§þºj¦ÚrK©j·!÷«q¿ìÏÛéÛ¦§ú`m§$º«r¯z·"×±íþÚÑý\{¾¼áÖøk^Ú×M:ã¿²·?n)ÿ¦né¶êZÈb½êÜûïþÊÜý¸§þºj¦ÚrK©j·!÷«s7ó¯·ó5ñº(È^rDzAH,Gé¦
~Ûi¢Ï¬¶»¶ËZ¶Ë~ûð¡yÉ"~Øb²+µêæv¥uë®*m¢¿þø¬jÛr~)^éú·º¹ßj[ûï¿
'í+"±©îÇ¦åyÈZ§-z»)yÈ×¯Ç¨®©{ÿ¢}û-j×âéè~-! FËl¶j{äHR:Ël¶j{êÞ¶êç}©l{ïÂç$¶~×¥(®K(Øb±ø¥{ûi¢Àµée¡ú]÷àÖ¿²·?w%¹×¿¦íÿ§ßjh®Ø[þÊÜþ)Üç^þ+·ü"}©¢»aw^Çoz÷N[s§<÷gtm¾4×]4ëþÊÜþ)Üç^þ+·ü"}©¢»aûïþÊÜþ)Üç^þ+·ü"}©¢»aßozû}½×g±µêç{i[-jÞ¸rìµÈZ®v¦zËk¹Ëlµ«[¹ø×Hg^~)ÞHi¦HLL TDéÝÿ¢~w^}"Nù×w!!Hé¦HLL R;ç§v'þùÝyôH!Ñù×w! Fi S!ÑùéÝÿÈÝz·è®[-jÛhmæëhêh®ØîËb¢zh{ljË2X§Ìv'ß+ZþÊÜþ+·ü"}¬µ«\oû+súh®ßð}ö²Ör)Ý{ºçÞãMôí¦ô÷ÍÚïtÓ®8kû+súh®ßð}ö²Ösï¾oû+súh®ßð}ö²Ösnzß¶ç®¹¦-jÞ¸rìµÈZ®v¦zËk¹Ëlµ«[¹ø§¶²Öë§µø¥zz,¶»¶ËZµ»C,HEWC,H¶©ÝçâéèÀ42ÄÄ ÑNDÀ ã_6káÀ42ÄX¥x42Äëh±øZÙ^~)^XäC~)^O*^R6CJ4åø5D9Z²Ñ+®þº{"w²+¶Ëf¡×¢~b Õ@,ÇÐËPPFî|P²þøEW²C,E@-A!)^42ÄÙ»,·«®zUºÞ¶êç×ð¡yÉ"~Ø^~)^¬iÖjËky©~Ê.W¬²+ajÆÞzzÞv*Þr×¶W zÛb§~æjØ¨Øk¢è!ÛÐË¥ö¢ÊéZµè²ËZ¶)ß¢¹¶*'{azj,µªi®k«^Á¬ÚÞ¶êçØ§²×wçâäò¥àzÑbäò¥èEW¾«-ºè¬´¶¬´Jë¢¿¾ÿâ'¾ëKjËD®º+z+uêí¡Ø¬¶)àº+!mëpyéÚ½©bwêÞ¶êç¡ñH,DØ

Re: pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

3 matches

Site Navigation

Mail list logo

Footer information