Re: [PATCH 3/4] Use get_remote_packet_size in download_tracepoint

[Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com>]

Ulrich Weigand <uweigand@de.ibm.com> writes:

> You know from the beginning that the agent expression will take
> (2 * aexpr->len) bytes, so it should be OK to only check this
> once, ahead of time.  In fact, sending a partial agent expression
> seems to be worse than sending none, so if the agent expression
> is too long, I think it should be just omitted (and the user
> warned).

I don't think a partial agent expression would be sent in this case,
since this is before the first putpkt is called in the function. But I
can still issue the warning and ignore the condition expression instead
of failing on the assertion. Otherwise I can check the size once and
call a gdb_assert if its too small, like the rest of the function. Which
is better?

Would something like one of the two alternative below be ok for checking
the size only once?

The second one looks complicated, but my goal was to avoid overflows in
2 * aexpr->len, since that length ultimately comes from the condition
expression the user supplies.

I am also assuming throughout this function that size_t and
gdb::char_vector::size_type are compatible (since buf.size () returns
the latter and xsnprintf takes a size_t). Is this ok?

Thanks!

1:

       if (remote_supports_cond_tracepoints ())
 	{
 	  agent_expr_up aexpr = gen_eval_for_expr (tpaddr, loc->cond.get ());
-	  xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":X%x,",
-		     aexpr->len);
-	  pkt = buf + strlen (buf);
-	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
-	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
-	  *pkt = '\0';
+
+	  int cond_str_size = snprintf (NULL, 0, ":X%x,", aexpr->len);
+	  gdb_assert (cond_str_size >= 0);
+
+	  cond_str_size += aexpr->len * 2;
+
+	  if (cond_str_size < buf.size () - strlen (buf.data ()))
+	    {
+	      sprintf (buf.data () + strlen (buf.data ()),
+		       ":X%x,", aexpr->len);
+
+	      pkt = buf.data () + strlen (buf.data ());
+
+	      for (int ndx = 0; ndx < aexpr->len; ++ndx)
+		pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
+	      *pkt = '\0';
+	    }
+	  else
+	    warning (_("Condition expression too long, "
+		       "ignoring tp %d cond"), b->number);
 	}
       else
 	warning (_("Target does not support conditional tracepoints, "

2:

       if (remote_supports_cond_tracepoints ())
 	{
 	  agent_expr_up aexpr = gen_eval_for_expr (tpaddr, loc->cond.get ());
-	  xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":X%x,",
-		     aexpr->len);
-	  pkt = buf + strlen (buf);
-	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
-	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
-	  *pkt = '\0';
+
+	  int cond_str_size = snprintf (NULL, 0, ":X%x,", aexpr->len);
+	  gdb_assert (cond_str_size >= 0);
+
+	  size_t size_left = buf.size () - strlen (buf.data ());
+
+	  bool size_ok = (cond_str_size < size_left);
+
+	  if (size_ok)
+	    {
+	      size_left -= cond_str_size;
+
+	      size_ok = (size_left/2 > aexpr->len);
+
+	      if (size_ok)
+		{
+		  sprintf (buf.data () + strlen (buf.data ()),
+			   ":X%x,", aexpr->len);
+
+		  pkt = buf.data () + strlen (buf.data ());
+
+		  for (int ndx = 0; ndx < aexpr->len; ++ndx)
+		    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
+		  *pkt = '\0';
+		}
+	    }
+
+	  if (!size_ok)
+	    warning (_("Condition expression too long, "
+		       "ignoring tp %d cond"), b->number);
 	}