Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Edit atoe_sprintf to handle arguments of any length #6601

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

WIP: Edit atoe_sprintf to handle arguments of any length #6601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 102 additions & 31 deletions util/a2e/atoe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1249,27 +1249,40 @@ int
atoe_fprintf(FILE *file, const char *ascii_chars, ...)
{
va_list args;
char buf[BUFLEN];
char *ebuf;
int len;
char buf[BUFLEN] = {0};
char *ebuf = NULL;
int len = 0;

va_start(args, ascii_chars);

len = atoe_vsnprintf(buf, BUFLEN, ascii_chars, args);

va_end(args);
/* Abort if failed... */
if (len == -1) {
if (-1 == len) {
return len;
}
} else if (len >= BUFLEN) {
/* Add 1 to accommodate for the null terminating char. */
int _len = len + 1;
char *_buf = (char *)malloc(_len);
va_start(args, ascii_chars);

len = atoe_vsnprintf(_buf, _len, ascii_chars, args);

ebuf = a2e(buf, len);
va_end(args);
if (-1 == len) {
return len;
}
ebuf = a2e(_buf, len);
free(ebuf);
} else {
ebuf = a2e(buf, len);
}
#pragma convlit(suspend)
len = fprintf(file, "%s", ebuf);
#pragma convlit(resume)
free(ebuf);

va_end(args);

return len;
}

Expand All @@ -1284,27 +1297,40 @@ int
atoe_printf(const char *ascii_chars, ...)
{
va_list args;
char buf[BUFLEN];
char *ebuf;
int len;
char buf[BUFLEN] = {0};
char *ebuf = NULL;
int len = 0;

va_start(args, ascii_chars);

len = atoe_vsnprintf(buf, BUFLEN, ascii_chars, args);

va_end(args);
/* Abort if failed... */
if (len == -1) {
if (-1 == len) {
return len;
}
} else if (len >= BUFLEN) {
/* Add 1 to accommodate for the null terminating char. */
int _len = len + 1;
char *_buf = (char *)malloc(_len);
va_start(args, ascii_chars);

len = atoe_vsnprintf(_buf, _len, ascii_chars, args);

ebuf = a2e(buf, len);
va_end(args);
if (-1 == len) {
Copy link
Contributor

@joransiu joransiu Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_buf would need to be freed on this path prior to the return statement. There are similar cases in the other functions you updated.

return len;
}
ebuf = a2e(_buf, len);
free(_buf);
} else {
ebuf = a2e(buf, len);
}
#pragma convlit(suspend)
len = printf("%s", ebuf);
#pragma convlit(resume)
free(ebuf);

va_end(args);

return len;
}

Expand Down Expand Up @@ -1341,8 +1367,8 @@ std_sprintf(const char *buf, char *ascii_chars, ...)
int
atoe_sprintf(char *buf, char *ascii_chars, ...)
{
int len;
char wrkbuf[BUFLEN];
int len = 0;
char wrkbuf[BUFLEN] = {0};

va_list args;
va_start(args, ascii_chars);
Expand All @@ -1352,10 +1378,24 @@ atoe_sprintf(char *buf, char *ascii_chars, ...)
va_end(args);
if (-1 == len) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strcpy((char *)buf, wrkbuf);

Will the strcpy segfault if buf == NULL (user-input)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, even with your suggested implementation for reducing performance costs, we will at least have to grab the length of the incoming arguments to decide if the "fast-path" option would fail

Not necessarily. See https://linux.die.net/man/3/vsnprintf.

The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated. (See also below under NOTES.)

Please confirm if the above statement is also true for our implementation.

If so, then the following should work:

len = atoe_vsnprintf(wrkbuf, BUFLEN, ascii_chars, args);
if (len >= BUFLEN) {
   /* Truncated: take the slow path. */
}

Size of wrkbuf must be defined before the call to vsnprintf, this is why we grab the length of the incoming string before creating the char array. If the conditional you suggest passes, we would create another char array with the correct size. Is this acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #6601 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it helps or not but with my changes to atoe_vsnprintf in this1 PR, it would behave more like std vsnprintf! for instance if you call it like atoe_vsnprintf(NULL, 0, format, arguments) it would return estimate of how much space that operation would take without actually write to any buffer! (it was returning -1 before my PR)

return len;
} else if (len >= BUFLEN) {
/* Add 1 to accommodate for the null terminating char. */
int _len = len + 1;
char *_wrkbuf = (char *)malloc(_len);
va_start(args, ascii_chars);

len = atoe_vsnprintf(_wrkbuf, _len, ascii_chars, args);

va_end(args);
if(-1 == len) {
return len;
}
strcpy((char *)buf, _wrkbuf);
free(_wrkbuf);
} else {
strcpy((char *)buf, wrkbuf);
}

strcpy((char *)buf, wrkbuf);

return len;
}

Expand Down Expand Up @@ -1406,17 +1446,31 @@ atoe_vprintf(const char *ascii_chars, va_list args)
int
atoe_vfprintf(FILE *file, const char *ascii_chars, va_list args)
{
char buf[BUFLEN];
char *ebuf;
int len;

len = atoe_vsnprintf(buf, BUFLEN, ascii_chars, args);
char buf[BUFLEN] = {0};
char *ebuf = NULL;
int len = 0;
va_list _args;
va_copy(_args, args);

if (len == -1) {
len = atoe_vsnprintf(buf, BUFLEN, ascii_chars, _args);

va_end(_args);
if (-1 == len) {
return len;
}
} else if (len >= BUFLEN) {
/* Add 1 to accommodate for the null terminating char. */
int _len = len + 1;
char *_buf = (char *)malloc(_len);

len = atoe_vsnprintf(_buf, _len, ascii_chars, args);

ebuf = a2e(buf, len);
if (-1 == len) {
return len;
}
ebuf = a2e(_buf, len);
} else {
ebuf = a2e(buf, len);
}
#pragma convlit(suspend)
len = fprintf(file, "%s", ebuf);
#pragma convlit(resume)
Expand All @@ -1435,14 +1489,31 @@ atoe_vfprintf(FILE *file, const char *ascii_chars, va_list args)
int
atoe_vsprintf(char *target, const char *ascii_chars, va_list args)
{
char buf[BUFLEN]; /*ibm@029013*/
char buf[BUFLEN] = {0}; /*ibm@029013*/
int bsize = 0; /*ibm@029013*/
va_list _args;
va_copy(_args, args);

bsize = atoe_vsnprintf(buf, BUFLEN, ascii_chars, _args); /*ibm@029013*/

bsize = atoe_vsnprintf(buf, BUFLEN, ascii_chars, args); /*ibm@029013*/
va_end(_args);
if (-1 == bsize) {
return bsize;
} else if (bsize >= BUFLEN) {
/* Add 1 to accommodate for the null terminating char */
int _bsize = bsize + 1;
char *_buf = (char *)malloc(_bsize);

bsize = atoe_vsnprintf(_buf, _bsize, ascii_chars, args);

if (-1 == bsize) {
return bsize;
}
strcpy(target, _buf);
free(_buf);
} else {
strcpy(target, buf); /*ibm@029013*/
}
strcpy(target, buf); /*ibm@029013*/
return bsize; /*ibm@029013*/
}

Expand Down
7 changes: 5 additions & 2 deletions util/a2e/atoe_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2001, 2019 IBM Corp. and others
* Copyright (c) 2001, 2022 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -417,6 +417,7 @@ atoe_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
bool_t long_flag, long_long_flag; /*ibm@9094*/
bool_t fPrecision;
int min_width, precision, ch;
int final_len = 0;
static char NULLCHARSTRING[] = "[null]"; /*ibm@029013*/
/*ibm@029013*/
if (fmt == NULL) { /*ibm@029013*/
Expand Down Expand Up @@ -496,6 +497,7 @@ atoe_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
goto next_char;
case 's':
strvalue = va_arg(args, char *);
final_len += (int)strlen(strvalue);
Copy link
Contributor

@joransiu joransiu Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to review and handle the final_len calculations for all the other cases besides "%s".

CheckRet(fstring(&this, strvalue, left_justify,
min_width, precision));
break;
Expand Down Expand Up @@ -581,11 +583,12 @@ atoe_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
return ERROR_RETVAL;
}
} else {
final_len++;
CheckRet(pchar(&this, ch));
}
}
*this.buffer = '\0';
return strlen(str);
return final_len;
}

/* END OF FILE */