su: Fix su - regression

[stoeckmann]

Launch a login shell again if requested through "su -" or "su -l".

Fixes: d992343 ("src/: Use xasprintf() instead of its pattern")
Closes: #1160

[glangshaw]

Launch a login shell again if requested through "su -" or "su -l".

Fixes: d992343 ("src/: Use xasprintf() instead of its pattern") Closes: #1160

Instead, why not just :
xasprintf(&cp, "-%s", cp);

and remove the arg0 .

[stoeckmann]

Should work. Is there a guarantee somewhere that vasprintf always has to properly handle situations where first argument is also part of the variable argument list?

Since I didn't find this guarantee now, I'll let maintainers decide and keep it as it is now to highlight the regression without doing more refactoring.

[glangshaw]

Fair enough. I just noticed that cp was being reused throughout that section of code, so it seemed in keeping with the general flow. In this case, the first parameter is the address of the cp pointer, while the 3rd parameter is the value of the cp pointer so I don't believe it will be a problem here. That said, the stdarg variadic stuff in C has always struck me as a bit weird and kludgy. I usually try and avoid using it in my own code. wherever possible.

Anyway, thanks for PR and the response,. As you say,, the shadow guys can decide for themselves. In all honesty, I preferred the old code before they switched to xasprintf().

[stoeckmann]

In this case, the first parameter is the address of the cp pointer, while the 3rd parameter is the value of the cp pointer

Oh right, it's call by value for the var args. In that case your suggestion works as well. Thanks for clarification.

[alejandro-colomar]

Should work. Is there a guarantee somewhere that vasprintf always has to properly handle situations where first argument is also part of the variable argument list?

It seems glibc is missing a access(write_only, 1) attribute.

$ grepc vasprintf /usr/include/
/usr/include/stdio.h:extern int vasprintf (char **__restrict __ptr, const char *__restrict __f,
		      __gnuc_va_list __arg)
     __THROWNL __attribute__ ((__format__ (__printf__, 2, 0))) __wur;

It should have __attribute__((access(write_only, 1))), I think.

I'll CC a few maintainers (we should also email their mailing list, but I'll CC them here for completeness).

Cc: @fweimer-rh , @eggert

[alejandro-colomar]

@hallyn should we release from master? Since we haven't yet applied any commits to master, I think this would be simpler.

[alejandro-colomar]

Reviewed-by: Alejandro Colomar <[email protected]>

[alejandro-colomar]

Since I didn't find this guarantee now, I'll let maintainers decide and keep it as it is now to highlight the regression without doing more refactoring.

Agree. Your patch better shows the regression. I'll merge now.

Thanks!

[hallyn]

Sorry what does "release from master" mean @alejandro-colomar

[alejandro-colomar]

Sorry what does "release from master" mean @alejandro-colomar

Should you release 4.17.1 just like you do with the .0 releases? Or should I create a 4.17.x branch and release 4.17.1 from there?

That is, the commit that changes the version number should be on a branch. I would put that commit in the master branch, for simplicity. Would you do the honours?

[hallyn]

Oh. I can't think of a particular advantage to either one.

[hallyn]

Sure I can do from master branch.

[hallyn]

(seems we should have a testcase for this, using expect...)

[alejandro-colomar]

Launch a login shell again if requested through "su -" or "su -l".
Fixes: d992343 ("src/: Use xasprintf() instead of its pattern") Closes: #1160

Instead, why not just : xasprintf(&cp, "-%s", cp);

and remove the arg0 .

On the API design space, I don't like this API. I can't blame the authors, since it was designed a long time ago, way before the malloc() GNU attribute was invented (I think), and before static analysis was so powerful.

These days, it would have been better to return a pointer to the newly allocated string, just like strdup(3). That would allow a much better static analysis. It also makes it obvious that the old value of the pointer is unimportant and ignored. Since most callers don't care about the length, but some may, I would have a size_t *len argument which can be NULL to ignore, or can be a pointer to a size_t for getting the length of the new string.

That would result in cp = xasprintf2(NULL, "-%s", cp);, which I think is cleaner.

[alejandro-colomar]

Launch a login shell again if requested through "su -" or "su -l".
Fixes: d992343 ("src/: Use xasprintf() instead of its pattern") Closes: #1160

Instead, why not just : xasprintf(&cp, "-%s", cp);
and remove the arg0 .

On the API design space, I don't like this API. I can't blame the authors, since it was designed a long time ago, way before the malloc() GNU attribute was invented (I think), and before static analysis was so powerful.

These days, it would have been better to return a pointer to the newly allocated string, just like strdup(3). That would allow a much better static analysis. It also makes it obvious that the old value of the pointer is unimportant and ignored. Since most callers don't care about the length, but some may, I would have a size_t *len argument which can be NULL to ignore, or can be a pointer to a size_t for getting the length of the new string.

That would result in cp = xasprintf2(NULL, "-%s", cp);, which I think is cleaner.

Hmmm, I would go further and say that those who care about the lenght, would avoid asprintf(3) altogether, and use some faster APIs. And if one wants the length ocasionally, they can just call strlen(3); the performance loss by calling strlen(3) is negligible after a call to asprintf(3). So a good API would just omit the length at all, and just return the newly allocated string.

New API in the oven, cooking while listening to the New Year's Concert by the Vienna Philharmonic. :)