Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()

[Lars Schneider]

> On 10 Aug 2016, at 15:37, Jeff King  wrote:
> 
> On Wed, Aug 10, 2016 at 03:29:26PM +0200, Lars Schneider wrote:
> 
>> 
>>> On 10 Aug 2016, at 15:15, Jeff King  wrote:
>>> 
>>> On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschnei...@gmail.com wrote:
>>> 
 From: Lars Schneider 
 
 format_packet() dies if the caller wants to format a packet larger than
 LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
>>> 
>>> I am not sure I agree here. Certainly I see the usefulness of gently
>>> handling a failure to write(). But if you are passing in too-large
>>> buffers, isn't that a bug in the program?
>>> 
>>> How would you recover, except by splitting up the content? That might
>>> not be possible depending on how you are using the pkt-lines. And even
>>> if it is, wouldn't it be simpler to split it up before sending it to
>>> format_packet()?
>> 
>> Good argument. I agree - this patch should be dropped.
> 
> Actually, after reading further, one thought did occur to me. Let's say
> you are writing to a smudge filter, and one of the header packets you
> send has the filename in it. So you might do something like:
> 
>  if (packet_write_fmt_gently(fd, "filename=%s", filename) < 0) {
>   if (filter_required)
>   die(...);
>   else
>   return -1; /* we tried our best; skip smudge */
>  }
> 
> The "recovery" there is not to try sending again, but rather to give up.
> And that is presumably a sane outcome for somebody who tries to checkout
> a filename larger than 64K.

Yes!


> It does still feel a little weird that you cannot tell the difference
> between a write() error and bad input. Because you really might want to
> do something different between the two. Like:
> 
>  #define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen("filename"))
> 
>  if (filename > MAX_FILENAME) {
>   warning("woah, that name is ridiculous; truncating");
>   ret = packet_write_fmt_gently(fd, "%.*s", MAX_FILENAME, filename);
>  } else
>   ret = packet_write_fmt_gently(fd, "%s", filename);


I can do that. However, I wouldn't truncate the filename as this
might create a weird outcome. I would just let the filter fail.

OK?

- Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()

[larsxschneider]

From: Lars Schneider 

format_packet() dies if the caller wants to format a packet larger than
LARGE_PACKET_MAX. Certain callers might prefer an error response instead.

Add a parameter `gentle` to define if the function should signal an error
with the return value (gentle=1) or die (gentle=0).

Signed-off-by: Lars Schneider 
---
 pkt-line.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 9400b47..e6b8410 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -108,7 +108,7 @@ static void set_packet_header(char *buf, const int size)
#undef hex
 }
 
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static int format_packet(int gentle, struct strbuf *out, const char *fmt, 
va_list args)
 {
size_t orig_len, n;
 
@@ -117,10 +117,15 @@ static void format_packet(struct strbuf *out, const char 
*fmt, va_list args)
strbuf_vaddf(out, fmt, args);
n = out->len - orig_len;
 
-   if (n > LARGE_PACKET_MAX)
-   die("protocol error: impossibly long line");
+   if (n > LARGE_PACKET_MAX) {
+   if (gentle)
+   return -1;
+   else
+   die("protocol error: impossibly long line");
+   }
 
set_packet_header(>buf[orig_len], n);
+   return 0;
 }
 
 void packet_write(int fd, const char *fmt, ...)
@@ -130,7 +135,7 @@ void packet_write(int fd, const char *fmt, ...)
 
strbuf_reset();
va_start(args, fmt);
-   format_packet(, fmt, args);
+   format_packet(0, , fmt, args);
va_end(args);
packet_trace(buf.buf + 4, buf.len - 4, 1);
write_or_die(fd, buf.buf, buf.len);
@@ -141,7 +146,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, 
...)
va_list args;
 
va_start(args, fmt);
-   format_packet(buf, fmt, args);
+   format_packet(0, buf, fmt, args);
va_end(args);
 }
 
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()

[Lars Schneider]

> On 10 Aug 2016, at 15:15, Jeff King  wrote:
> 
> On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider 
>> 
>> format_packet() dies if the caller wants to format a packet larger than
>> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
> 
> I am not sure I agree here. Certainly I see the usefulness of gently
> handling a failure to write(). But if you are passing in too-large
> buffers, isn't that a bug in the program?
> 
> How would you recover, except by splitting up the content? That might
> not be possible depending on how you are using the pkt-lines. And even
> if it is, wouldn't it be simpler to split it up before sending it to
> format_packet()?

Good argument. I agree - this patch should be dropped.

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()

[Jeff King]

On Wed, Aug 10, 2016 at 03:29:26PM +0200, Lars Schneider wrote:

> 
> > On 10 Aug 2016, at 15:15, Jeff King  wrote:
> > 
> > On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschnei...@gmail.com wrote:
> > 
> >> From: Lars Schneider 
> >> 
> >> format_packet() dies if the caller wants to format a packet larger than
> >> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
> > 
> > I am not sure I agree here. Certainly I see the usefulness of gently
> > handling a failure to write(). But if you are passing in too-large
> > buffers, isn't that a bug in the program?
> > 
> > How would you recover, except by splitting up the content? That might
> > not be possible depending on how you are using the pkt-lines. And even
> > if it is, wouldn't it be simpler to split it up before sending it to
> > format_packet()?
> 
> Good argument. I agree - this patch should be dropped.

Actually, after reading further, one thought did occur to me. Let's say
you are writing to a smudge filter, and one of the header packets you
send has the filename in it. So you might do something like:

  if (packet_write_fmt_gently(fd, "filename=%s", filename) < 0) {
if (filter_required)
die(...);
else
return -1; /* we tried our best; skip smudge */
  }

The "recovery" there is not to try sending again, but rather to give up.
And that is presumably a sane outcome for somebody who tries to checkout
a filename larger than 64K.

It does still feel a little weird that you cannot tell the difference
between a write() error and bad input. Because you really might want to
do something different between the two. Like:

  #define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen("filename"))

  if (filename > MAX_FILENAME) {
warning("woah, that name is ridiculous; truncating");
ret = packet_write_fmt_gently(fd, "%.*s", MAX_FILENAME, filename);
  } else
ret = packet_write_fmt_gently(fd, "%s", filename);

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()

[Jeff King]

On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> format_packet() dies if the caller wants to format a packet larger than
> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.

I am not sure I agree here. Certainly I see the usefulness of gently
handling a failure to write(). But if you are passing in too-large
buffers, isn't that a bug in the program?

How would you recover, except by splitting up the content? That might
not be possible depending on how you are using the pkt-lines. And even
if it is, wouldn't it be simpler to split it up before sending it to
format_packet()?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()

[Jeff King]

On Wed, Aug 10, 2016 at 03:59:19PM +0200, Lars Schneider wrote:

> > It does still feel a little weird that you cannot tell the difference
> > between a write() error and bad input. Because you really might want to
> > do something different between the two. Like:
> > 
> >  #define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen("filename"))
> > 
> >  if (filename > MAX_FILENAME) {
> > warning("woah, that name is ridiculous; truncating");
> > ret = packet_write_fmt_gently(fd, "%.*s", MAX_FILENAME, filename);
> >  } else
> > ret = packet_write_fmt_gently(fd, "%s", filename);
> 
> 
> I can do that. However, I wouldn't truncate the filename as this
> might create a weird outcome. I would just let the filter fail.

Yeah, I think that is probably fine (I don't have a real opinion for
this particular case, but was mostly just trying to think about whether
the pktline interface was suitably flexible).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html